-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: support eks endpoints and auth token file in container creds #1093
Conversation
A new generated diff is ready to view.
|
A new generated diff is ready to view.
|
// TODO - resolve hostnames | ||
is Host.Domain -> throw ProviderConfigurationException( | ||
"The container credentials full URI ($uri) is specified via hostname which is not currently supported.", | ||
) |
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.
Question: Leaving aside the question of full hostname resolution, is it not safe to accept "localhost"
here?
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.
localhost
can be remapped. Obviously at that point you already have bigger issues if your attacker has root access to be able to do such things.
We have a ticket for this #476
@@ -81,7 +78,7 @@ public class EcsCredentialsProvider( | |||
} | |||
|
|||
val op = SdkHttpOperation.build<Unit, Credentials> { | |||
serializer = EcsCredentialsSerializer(authToken) | |||
serializer = EcsCredentialsSerializer(loadAuthToken()) |
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.
style: I would have left this suspend call as an assignment to authToken
as it was
// TODO - resolve hostnames | ||
is Host.Domain -> throw ProviderConfigurationException( | ||
"The container credentials full URI ($uri) is specified via hostname which is not currently supported.", | ||
) |
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.
localhost
can be remapped. Obviously at that point you already have bigger issues if your attacker has root access to be able to do such things.
We have a ticket for this #476
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
A new generated diff is ready to view.
|
Description of changes
AWS_CONTAINER_CREDENTIALS_FULL_URI
AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE
which is queried on every credentials retrieval for its auth token.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.