fix(storage): AWS Dynamic Credentials (Assume Role)#33
Conversation
hugoghx
force-pushed
the
hugo-assume-role
branch
3 times, most recently
from
1155af2 to
e908091
Compare
hugoghx
force-pushed
the
hugo-assume-role
branch
from
e55d673 to
30d9082
Compare
This comment was marked as resolved.
This comment was marked as resolved.
ddebko
left a comment
ddebko
left a comment
There was a problem hiding this comment.
This looks great. I think there is one edge case we didn't cover for updating a storage bucket, which is deleting the old bucket's static credentials that we are managing with credential rotation. This should be done after we have checked that the new credentials dry run validation has passed and we have confirmed that the keys can be rotated if credential rotation is enabled.
| StaticOther | ||
|
|
||
| // Unknown is a catch-all for everything else. | ||
| Unknown |
There was a problem hiding this comment.
What would the behavior of unknown be?
There was a problem hiding this comment.
For the most part, it's used to carve out a special condition (eg: ToMap), fast-track the env variables use case, or an error, since we don't really know what it is.
| // StaticOther denotes the presence of an Access Key Id that does not follow | ||
| // the AKIA/ASIA convention outlined by AWS. | ||
| StaticOther |
There was a problem hiding this comment.
I am curious if this could ever cause problems if returned by GetCredentialType since there are a few checks for StaticAWS specifically, or is this just for service providers other than AWS?
There was a problem hiding this comment.
or is this just for service providers other than AWS?
Yep, this is exactly what it is for - StaticAWS uses the hard-coded AWS conventions, and then everything else that doesn't follow that is StaticOther, for S3-Compatible... compatibility 😂
There was a problem hiding this comment.
Have we tested a S3 compatible service with these code changes? Other than aws S3.
There was a problem hiding this comment.
I have not - I think that's outside the scope of this work since this patch isn't really about enabling S3-compatible services. The StaticOther credential type is introduced here more as a way of keeping that path open for future S3-Compatibility work and not shoot ourselves in the foot.
I might spin up a MinIO instance and give it a quick shot though 😄
ddebko
left a comment
ddebko
left a comment
There was a problem hiding this comment.
I'm happy to approve this PR as long as we have a follow up task for a later date to resolve the issue of orphaned secrets that boundary manages when invoking update storage bucket.
Yep, that's already set-up :) |
|
Merging this PR onto |
This commit introduces fixes to enable operators to use AWS dynamic credentials on the storage side of this plugin, namely validation, improved credential lifecycle management to allow for storage buckets to be updated from static to dynamic credentials and vice versa, and a new way to determine credential types. It also introduces new tests for dynamic credential usage for the various functions.
On Storage Bucket update, when we determine if the credentials are to be updated to new ones, we call `credState.ReplaceCreds`. This call, if the old credentials were managed by us (rotated at create time), will delete the old credentials and set the new ones onto credState. We called this function without validating if the incoming credentials are valid. This could get the bucket in a state where where, while we'd return an update error if the incoming credentials weren't working with S3 (err in the dry run process that happens at the end of the update), we'd also silently delete the old credentials the bucket used, rendering the bucket completely broken.
hugoghx
force-pushed
the
hugo-assume-role
branch
from
df45fa4 to
f887665
Compare
4 participants
This PR introduces fixes to enable operators to use AWS dynamic credentials on the storage side of this plugin, namely validation, improved credential lifecycle management to allow for storage buckets to be updated from static to dynamic credentials and vice versa, and a new way to determine credential types.
It also introduces new tests for dynamic credential usage for the various functions.
Finally, it also fixes a problem with static rotated credentials being deleted from AWS when the plugin errored due to lack of validation