-
Notifications
You must be signed in to change notification settings - Fork 704
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(validation): Support AWS assumeRole and setting credentials at the command line #508
Conversation
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.
This looks like it could be ok, though I dont know much about aws to offer insights. I didnt look at the implementation details, just the interface.
While having encapsulated account specifications seems consistent with the other providers, personally I wish I could just give a standard credentials file at the aws level instead of having to give keys and secrets for each account. Especially since you could use that directly. From a security perspective, that means I need to protect lots of individual bits rather than protecting just a single file. I dont know what people do in practice, but for me just having a file and giving halyard that file would be much easier.
@@ -46,6 +46,12 @@ protected String getPersistentStoreType() { | |||
private String rootFolder; | |||
|
|||
@Parameter( | |||
names = "--acoount-name", |
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.
account-name
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.
Nice work, I think we're getting close! I'm going to take a second pass soon
private String assumeRole; | ||
|
||
@Parameter( | ||
names = {"--access-key-id", "--access-key"}, |
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 think there is a password = true
parameter that ensures these don't have to appear in your .bash_history
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.
It is, but it works a little bit too well... When true, JCommander no longer accepts the parameter at all, which makes scripting difficult. It displays an interactive prompt even if the parameter is provided.
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.
Yeah it sets the arity to 0, which can be confusing
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.
So, the conclusion is to not use password = true
, right?
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.
No, since that encourages putting secrets into your bash history.
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 it becomes a problem we should look at other ways to supply those secrets, but I think we should be wary of the security implications first
public class AwsAccount extends Account { | ||
|
||
private String defaultKeyPair; | ||
private String edda; | ||
private String discovery; | ||
private String accountId; | ||
private List<AwsRegion> regions = new ArrayList<>(); | ||
private String assumeRole; | ||
private AWSCredentials awsCredentials; |
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 don't think this should appear here, I don't know how it's serialized & this should be easily editable via yaml
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.
Fixed
@@ -25,6 +25,7 @@ | |||
@Data | |||
@EqualsAndHashCode(callSuper = false) | |||
public class S3PersistentStore extends PersistentStore { | |||
private String accountName; |
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.
Is this to avoid duplicating the secret key fields here? We originally took this approach with GCS but dropped it because it required granting our keys super extra privileges in clouddriver that weren't required for deploying infrastructure, only managing buckets
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 noticed this pattern somewhere else so I didnt say anything since I figured it was a standard pattern.
However I'd like to note that it could be that the account for the storage is not something that you want to deploy infrastructure to (other than perhaps Spinnaker). So you wouldnt necessarily want it visible in clouddriver.
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.
Agreed, we removed it from GCS for that reason
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.
Ok, will remove and replace with access key and secret key.
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.
What about assume role? Does that make sense 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.
I think so - you can have front50 do assume role style authentication, right?
docs/commands.md
Outdated
* `--account-id`: Your AWS account ID to manage. See http://docs.aws.amazon.com/IAM/latest/UserGuide/console_account-alias.html for more information. | ||
* `--assume-role, --role`: If set, Halyard will configure a credentials provider that use AWS Security Token Service to assume the specified role. Should either start with "arn:aws:iam::" or just the part after the last colon. |
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.
What is the role name used for, does the authentication mechanism use it, or is it just for our validation?
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.
Yes, it is used by the authentication mechanism.
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.
Nice
afd4c88
to
65fcde0
Compare
description = AwsCommandProperties.ACCOUNT_ID_DESCRIPTION | ||
names = "--account-id", | ||
description = AwsCommandProperties.ACCOUNT_ID_DESCRIPTION, | ||
required = true |
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.
Am I wrong in assuming that account ID is required?
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 think you're right here - can @gardleopard confirm?
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 guess the only thing I can think of is that we in some hypothetical scenario could use AWS account lookup to find the account id, but I don't think we should support that.
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 tested with some local configs and it seems like account id is required.
65fcde0
to
d477123
Compare
d477123
to
4e636cc
Compare
Second take at implementing validation of AWS accounts and S3 buckets. Please take a look @lwander @ewiseblatt.
It kind of supports the assumeRole, and you can set credentials in the cli. It's not properly tested, so expect the unexpected. I just wanted to get it out there and collect some feedback.