docs(auth): add warning about externally-provided credentials #11462
Conversation
quartzmo
force-pushed
the
auth-warning
branch
from
65ef727 to
38ec6c9
Compare
quartzmo
added
the
do not merge
| @@ -109,18 +109,50 @@ type CredentialSource struct { | |||
| // File is the location for file sourced credentials. | |||
| // One field amongst File, URL, Executable, or EnvironmentID should be | |||
| // provided, depending on the kind of credential in question. | |||
| // | |||
| // Important: If you accept a credential configuration (credential | |||
There was a problem hiding this comment.
At least for this package we have a security considerations section in the godoc -- perhaps this should be added there. This actually sounds pretty similar to that disclaimer. Wonder if we should just have that sort of message in each of the credential packages instead of putting this on the field docs.
There was a problem hiding this comment.
@sai-sunder-s PTAL at Cody's comment above.
There was a problem hiding this comment.
We mainly need it for the method that loads any arbitrary json. This is what we are doing in other languages.
@codyoss what do you mean by "each credential package"? I thought go doesn't expose per credential type methods?
There was a problem hiding this comment.
The existing warning is in external account specific section. Do you mean put the warning in the documentation of each cred?
If so, that is not of much use for the issue we are dealing with now. The issue is specifically when a developer accepts cred config from external sources and builds a cred out of it. They expect only certain type like Service Account but they end up accepting other cred types.
So the warning needs to be in a central place like the method used to load arbitrary creds. Maybe it can go at the top of the detect method as well?
There was a problem hiding this comment.
I think what I was aiming for was having a central section to put such information. I don't love having to maintain the same docs in N places. Would prefer to have it in a single place. I was wondering if having a security section in the package docs would suffice.
There was a problem hiding this comment.
I think it needs to be there for the detect method. Additionally can be put in central place as well. We can remove other places added in this PR.
Does that sound ok?
There was a problem hiding this comment.
we can remove the warnings added to this file
There was a problem hiding this comment.
We can remove other places added in this PR.
Please directly note on this PR which warnings to keep and which to remove.
| @@ -109,18 +109,50 @@ type CredentialSource struct { | |||
| // File is the location for file sourced credentials. | |||
| // One field amongst File, URL, Executable, or EnvironmentID should be | |||
| // provided, depending on the kind of credential in question. | |||
| // | |||
| // Important: If you accept a credential configuration (credential | |||
There was a problem hiding this comment.
we can remove the warnings added to this file
quartzmo
removed
the
do not merge
No description provided.