Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for
credential_process
from profiles #1356Add support for
credential_process
from profiles #1356Changes from 1 commit
c3a595c
f8b05ad
14abfdd
14ad7a9
b5f1c6b
865a6ee
a787b61
7de0201
2e0ae8f
0e4f34c
6433a24
9f88a6b
16a8495
ce75253
7b867af
3be6523
2a888d4
652345d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command binary name can be logged at
debug
, but the arguments must be logged attrace
since they can have sensitive information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of a non-zero exit status, the contents of
stderr
should be included in the returned error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should spawn this into a task...the trouble is that we'd need to then abstract over spawning. I'll think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'm open to thoughts here. I actually up refactoring this to take the Java SDK's approach of just passing the whole string through
sh -c
/cmd.exe /C
in 14ad7a9 to avoid needing to do any parsing in the SDK itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should spawn this into a task...the trouble is that we'd need to then abstract over spawning. I'll think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll need to figure out how to mock this—probably some sort of "fake command" interface will need to get added to our existing OS shim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, should validate that the credentials process can be used as the source profile for assume role
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I ended up using
echo
for the test in which should be supported on all platforms, but happy to try to sub in command mocking if you prefer.Ref: a787b61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
new
returned a result, we could return a profile file error here which is probably the right thingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in #1356 (comment) but I actually ended up pulling out the command parsing logic, following suit with the Java SDK of just passing through the commands to
sh -c
/cmd.exe /C
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
new
returned a result, we could return a profile file error here which is probably the right thingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arguments to the credentials process are allowed to contain sensitive information, so we need to be careful not to log them by default. Thus, the
Debug
implementation for this enum needs to do some amount of redaction on this value. I recommend creating a new-type around the&'a str
, and manually implementingDebug
for it such that only the part before the first space is output, followed by something like<args redacted>
if there was more to the string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this—we should make something like:
we can then use it here and in the provider to avoid needing to manually do a full debug impl