-
Notifications
You must be signed in to change notification settings - Fork 821
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
Metadata server running on windows #174
Metadata server running on windows #174
Conversation
I'm an experienced developer but this is the first Go code I've ever written, so assume I'm a complete newbie. I'm totally happy to have my approach/style critiqued |
One thing i didn't fully understand is why there are two servers running. One on 9099 which generates the security credentials payload and presumably token management and another than exposes it on the metadata ip address. Could the two not be combined? If they could be combined that would solve the nested calls |
I wish I'd documented it better, as I wrote it several years ago, but the two part design has some advantages. The part that fetches credentials and has access to the keyring is run in the foreground on localhost:9099 and only lasts as long as your interactive It's a stronger design from a security perspective, but it also makes it possible to prompt for MFA. I'd definitely consider keeping things simple and consolidating to a I don't mind the idea of requiring users to run it in another console tab. It takes away some of the magic and will perhaps mean people adopt it more. |
I generally dislike the fact that we call sudo ourselves anyway. We could drop that and leave it up to the user to use sudo as makes sense. |
I've tried automatically starting the https://github.com/99designs/aws-vault/blob/master/server/server.go#L73-L80 however exec is still expecting to execute another command and crashes when it cant this makes it happy, but clearly im missing something aws-vault exec playpen --server -- cmd /C pause now the process that is bound to port 80 is long lived and continues to run once started. Obviously this requires an elevated shell to run the first time (windows equivalent of sudo). I feel like I'm missing a key bit of knowledge about how the metadata server is supposed to be executed. |
So there are two http servers:
So with those two servers in place, the aws cli tools can request IAM creds, and refresh them as they go (refreshing them is important, is it keeps the lifetime low). The lines you referred to (https://github.com/99designs/aws-vault/blob/master/server/server.go#L73-L80) just check that the "proxy" is running, as the "credential" server isn't much use without it. Does that help? |
It seems like the hard part on windows is going to be the "proxy" server. I'd be tempted to require that windows users manually invoke the proxy server component: if !checkServerRunning(metadataBind) {
if runtime.GOOS == "windows" {
app.Fatal("The credential proxy server isn't running. Run aws-vault server as Administrator in the background and then try this command again")
}
log.Printf("Starting `aws-vault server` as root in the background")
cmd := exec.Command("sudo", "-b", os.Args[0], "server")
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
return err
}
} |
The more I think about it, I think the two part server design is better. It prevents people from running long lived credential servers, which basically negate the security value of aws-vault. |
Have we lost you @williamdenton? :) |
Sorry, no, just busy with another project currently, will return here shortly. Will try the suggestion above and update PR soon |
Yep think I agree @lox, 2 part makes a lot of sense. I did some googling, and on Windows, the runas command might work to run the command as Administrator, although I haven't tried this myself. I have a few suggestions on the UX around the server starting
|
the server will fail to launch unless in an administrative shell but the error is descriptive
eb47875
to
6fa9164
Compare
this solution has very similar behavior to other platforms now:
I think this is the best option for keeping the behaviour consistent on all platforms .\aws-vault.exe exec playpen -s -- aws s3 ls
Enter passphrase to unlock ...\.awsvault\keys:
Creation of network alias for server mode requires elevated permissions (Run as administrator).
aws-vault: error: Server failed: exit status 1
Unable to locate credentials. You can configure credentials by running "aws configure". the Because I'm using |
if the background server is not launched with administrative rights it will terminate immediately. There are two conditions this delay is hit: * if running in an elevated command prompt for the first time * not run in an elevated command prompt but it should have been So during normal operation this delay wont be encountered.
@lox this seems to get it functional and usable on windows. |
I'll review in detail shortly! Thanks so much @williamdenton! |
I wonder if we could use |
server/server.go
Outdated
cmd.Stderr = os.Stderr | ||
if err := cmd.Run(); err != nil { | ||
return err | ||
} |
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.
Might read a bit nicer if we pull this logic out into 2 functions, StartCredentialsServerOnWindows
and StartCredentialsServerWithSudo
? And might be worth doing the checkServerRunning
in both cases - there are failure scenarios if sudo doesn't exist for example
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 sudo doesn't exist then we still get to see the error as we start sudo with cmd.Run()
. This launches sudo and waits for it to finish - which it does as soon as it starts the process in the background -b
a couple of questions about merging: |
It's fine how it is! Basically as soon as it's a 👍 from two of us, we'll merge it. |
if runtime.GOOS == "windows" { | ||
return StartCredentialProxyOnWindows() | ||
} | ||
return StartCredentialProxyWithSudo() |
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.
@williamdenton Actually another way to do this is put the arch specific functions into conditional build files, rather than using the runtime check. What's preferred @lox ?
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 gets complicated, let's stick to this switch. LGTM.
Issue #166 describes whats going on here
This is a breaking change for the way the metadata server is started
is now