Add console subcommand #93

RowdyLemon · 2017-07-27T20:13:57Z

Vaulted console uses an access key to open an AWS console in the
default browser.

ryan-norton · 2017-07-31T16:49:05Z

console.go

+)
+
+var (
+	ErrInvalidTemporaryCredentials = errors.New("Invalid temporary session credentials present, permanent credentials are required.\nPermanent credentials can be used be specifying a vault name")


This message should probably be tweaked. First, "invalid temporary session credentials present" isn't accurate. It has nothing to do with the validity of the creds, they're simply not capable of invoking the console. Second, I think we can explain the permanent credentials / vault option a little cleaner.

ryan-norton · 2017-07-31T16:51:16Z

console.go

+
+var (
+	ErrInvalidTemporaryCredentials = errors.New("Invalid temporary session credentials present, permanent credentials are required.\nPermanent credentials can be used be specifying a vault name")
+	ErrInvalidDuration             = errors.New("Minimum console duration must be at least 15 min")


We should either not abbreviate "minutes" or abbreviate it more in line with other messages we post about duration - "15m"

ryan-norton · 2017-07-31T17:26:53Z

console.go

+			if err != nil {
+				return nil, nil, err
+			}
+			awsCreds, err = awsCreds.GetFederationToken(*fedDuration)


What if there are no AWS credentials in the vault?

ryan-norton · 2017-07-31T17:27:35Z

console.go

+			}
+		}
+	} else {
+		tempCreds, err := session.AWSCreds.GetLoadedOrDefaultCreds()


What happens here if it is unable to load default creds from the environment? Does it error, or does it simply set the values to empty string or nil?

ryan-norton · 2017-07-31T17:29:14Z

console.go

+		}
+
+		if c.Role != "" {
+			awsCreds, err = session.AWSCreds.AssumeRole(c.Role, *fedDuration)


Perhaps we can try to merge some of these checks / assumes? We seem to be repeating certain actions between vault and no-vault, I think we can wrap them a bit cleaner.

ryan-norton · 2017-07-31T17:29:51Z

console.go

+				return nil, nil, err
+			}
+		} else {
+			awsCreds, err = session.AWSCreds.GetFederationToken(*fedDuration)


What happens if we were unable to load credentials by this point?

ryan-norton · 2017-07-31T17:32:17Z

doc/vaulted-console.1.md

+A role to assume can be specified either in a vault's configuration (via
+`vaulted edit`) or specified via the `--assume` option.
+
+Vaulted first opens the specified vault to retrieve the appropriate credentials. If a role is specified in the vault's configuration it will use that unless a role is explicitely passed in through the `--asume` option. 


Couple typos:

--assume

explicitly

ryan-norton · 2017-07-31T17:36:00Z

lib/aws_credentials.go

 	"fmt"
+	// "io/ioutil"


Should this have been removed?

ryan-norton

Looks pretty good! Just a couple nits.

ryan-norton · 2017-09-29T00:44:02Z

console.go

+		}
+
+		return creds.GetSigninToken(&duration)
+


nit: line gap

ryan-norton · 2017-09-29T00:49:41Z

console.go

+}
+
+func (c *Console) getSigninToken(store vaulted.Store) (string, error) {
+	// Setup default values (may be overwritten by values from vault)


Future work: Perhaps we should allow this to be passed via command line as well?

ryan-norton · 2017-09-29T00:53:08Z

doc/vaulted-console.1.md

+
+Opens the AWS console in the default web browser. Uses either the credentials in the current environment or the credentials in the specified vault. Console sessions either use the provided vault's duration or defaults to 1 hour.
+
+Durations must be at least 15 min and less than 12 hours. 


nit: s/min/minutes

ryan-norton · 2017-10-27T16:32:38Z

command_test.go

@@ -445,6 +449,30 @@ var (
 			Args:    []string{"-V"},
 			Command: &Version{},
 		},
+		// Console


Do we have any tests for "duration"?

ryan-norton · 2017-10-27T16:44:10Z

console.go

+	}
+
+	// If duration was provided through the command line overwrite with that
+	if c.Duration != 0 && (c.Duration < 15*time.Minute || c.Duration > 12*time.Hour) {


Should we be checking if the Duration is valid when we receive the command, rather than in the console handling?

ryan-norton · 2017-10-27T17:41:07Z

console.go

+)
+
+const (
+	CONSOLE_URL = "https://console.aws.amazon.com/console/home"


We shouldn't be mixing UPPER_CASE and CamelCase types here.

ryan-norton · 2017-10-27T17:42:16Z

console.go

+	CONSOLE_URL = "https://console.aws.amazon.com/console/home"
+	SIGNIN_URL  = "https://signin.aws.amazon.com/federation"
+
+	MinConsoleDuration = 15 * time.Minute


These should probably begin with "Console" like the above consts, just to keep a consistent namespace.

ryan-norton · 2017-10-27T18:32:10Z

console.go

+		}
+	}
+
+	// If duration was provided through the command line overwrite with that


Also, 15*time.Minute and 12*time.Hour should be used as consts when we do these checks.

ryan-norton · 2017-10-27T18:44:13Z

console.go

+			if tokenErr != nil {
+				return "", tokenErr
+			}
+			creds, err = creds.AssumeRoleWithMFA(awsKey.MFA, tokenCode, awsKey.Role, 15*time.Minute)


15*time.Minute feels like it should be used as a const.

ryan-norton · 2017-10-27T18:44:20Z

console.go

+			}
+			creds, err = creds.AssumeRoleWithMFA(awsKey.MFA, tokenCode, awsKey.Role, 15*time.Minute)
+		} else {
+			creds, err = creds.AssumeRole(awsKey.Role, 15*time.Minute)


15*time.Minute feels like it should be used as a const.

ryan-norton · 2017-10-27T18:51:43Z

doc/vaulted.1.md

@@ -28,6 +28,9 @@ COMMANDS
 `add` / `create` / `new`
  Interactively creates the content of a new vault. See vaulted-add(1).

+`console`
+  Opens the AWS console in the default web browser. See vauled-console(1).


s/vauled/vaulted

This is still wrong.

ryan-norton · 2017-10-27T18:57:25Z

doc/vaulted-console.1.md

+DESCRIPTION
+-----------
+
+Opens the AWS console in the default web browser. Uses either the credentials in the current environment or the credentials in the specified vault. Console sessions either use the passed in duration, the provided vault's duration, or defaults to 1 hour.


I feel like this description can use a little work. For starters, "uses either the credentials in the current environment or the credentials in the specified vault" doesn't really capture the order of preference.

Second, "Console sessions use" seems unnecessary, given that this is a document on the console command, so the duration is implied to correspond to the console. Additionally, it feels a bit disjointed in how we are going about describing the methods by which a duration is supplied, made worse when we seem to take on the duration range after the rest.

Perhaps we should take a pass on this and see if we can improve the wording a bit?

ryan-norton

Interesting thing I noticed when I pulled this down to check something - I ran vaulted console on two different vaults containing credentials for two different AWS accounts. The first attempt worked, the second attempt went to an error page informing me that I was logged into a different account. We should see if this can be rectified, or at least document the case in which this is occurring. In this case I had already closed out of the console window for the other account, so it felt odd.

Also, were you able to apply any further error handling around the case where we receive permission errors?

ryan-norton · 2017-10-27T20:15:52Z

doc/vaulted.1.md

@@ -28,6 +28,9 @@ COMMANDS
 `add` / `create` / `new`
  Interactively creates the content of a new vault. See vaulted-add(1).

+`console`
+  Opens the AWS console in the default web browser. See vauled-console(1).


This is still wrong.

ryan-norton · 2017-10-27T20:20:39Z

command_test.go

@@ -544,6 +579,11 @@ var (
 			Args: []string{"upgrade", "one"},
 		},

+		// Console


You should alphabetize this with the rest of the commands.

ryan-norton · 2017-10-27T20:21:46Z

command_test.go

@@ -544,6 +579,11 @@ var (
 			Args: []string{"upgrade", "one"},
 		},

+		// Console
+		{
+			Args: []string{"Console", "one", "two"},


Why is Console capitalized here?

In our fail test cases here we should probably test --duration min/max values outside of the accepted range, like:

Args: []string{"console", "one", "--duration", "14m"},

ryan-norton · 2017-10-27T20:23:19Z

command_test.go

+			Args: []string{"console", "--duration", "15m", "one"},
+			Command: &Console{
+				VaultName: "one",
+				Duration:  ConsoleMinDuration,


It feels weird to specify it as "15m" in the args string and "ConsoleMinDuration" in the Command. The above string is meant to represent an entered command, so it seems wrong to then correlate their input to a const duration, particularly if the test isn't explicitly meant to test the ConsoleMinDuration (which, if this is, we need to be testing the ConsoleMaxDuration as well, including testing that values outside of those ranges do not work)

ryan-norton · 2017-10-27T20:35:48Z

command_test.go

@@ -445,6 +449,37 @@ var (
 			Args:    []string{"-V"},
 			Command: &Version{},
 		},
+		// Console


We should include this in the command list alphabetically.

ryan-norton · 2017-10-27T21:56:06Z

console.go

+	if err != nil {
+		if err != credentials.ErrNoValidProvidersFoundInChain {
+			return nil, err
+		} else if err == credentials.ErrNoValidProvidersFoundInChain || !awsCreds.Valid() {


err == credentials.ErrNoValidProvidersFoundInChain || is unnecessary, right? We effectively did this check in the above part of the if statement.

Plus, we're never actually checking !awsCreds.Valid() since we will trigger on the ErrNoValidProvidersFoundInChain piece.

ryan-norton · 2017-10-27T21:58:38Z

console.go

+		} else if err == credentials.ErrNoValidProvidersFoundInChain || !awsCreds.Valid() {
+			return nil, ErrNoCredentialsFound
+		}
+	} else if awsCreds.ValidSession() {


The else if statement here can probably be pulled out of the transition statement and just be checked before return awsCreds, nil at the end, right?

The above check confirms that the creds are Valid, but does not check if its a session, we should probably always be checking that.

ryan-norton · 2017-10-27T22:03:04Z

console.go

+}
+
+func (c *Console) getSigninToken(store vaulted.Store) (string, error) {
+	vault, err := c.getVault(store)


It feels like we really should not be storing and passing on a vault to everything in this function.

It feels like we should be calling OpenVault when necessary and pulling the requisite pieces (Key, Duration, etc) long enough to store those but not holding and sharing the vault with everything else to get their piece from it.

It feels like this level should have some sort of control flow on if c.VaultName != "", rather than doing this down in another method.

ryan-norton · 2017-10-27T22:03:53Z

console.go

+
+	awsKey := c.getAWSKey(vault)
+
+	awsCreds, err := c.getCredentials(vault, awsKey)


awsCreds feels wrong here. iirc You can use AWSKey.Credentials() to get the creds from a key, so we don't need to keep passing them along separately.

ryan-norton · 2017-10-27T22:04:52Z

console.go

+		duration = ConsoleDefaultDuration
+	}
+
+	return capDuration(duration)


Even if we've validated the Duration when we received the command, it still feels like we shouldn't be calling capDuration on the command flag duration. capDuration really only belongs with the Vault duration.

ryanmt

Drive-by ping, still wish I had this feature sometimes.

ryanmt · 2020-04-22T22:54:11Z

command.go

+	vaultName := ""
+	assume, _ := flag.GetString("assume")
+	duration, _ := flag.GetDuration("duration")
+	if duration != 0 && (duration < ConsoleMinDuration || duration > ConsoleMaxDuration) {


Console*Duration constants aren't specific to the console, but to the underlying Role. These should probably be named/refactored accordingly.

ryan-norton requested changes Jul 31, 2017

View reviewed changes

RowdyLemon force-pushed the consoleSubcommand branch 4 times, most recently from bdf1167 to bd677cb Compare August 1, 2017 20:45

RowdyLemon force-pushed the consoleSubcommand branch from bd677cb to f55532f Compare September 6, 2017 19:49

ryan-norton requested changes Sep 29, 2017

View reviewed changes

Add console subcommand

d34b43f

RowdyLemon force-pushed the consoleSubcommand branch from f55532f to d34b43f Compare October 23, 2017 20:22

ryan-norton requested changes Oct 27, 2017

View reviewed changes

RowdyLemon force-pushed the consoleSubcommand branch from 8957a3c to 08ccf83 Compare October 27, 2017 20:07

ryan-norton requested changes Oct 27, 2017

View reviewed changes

RowdyLemon force-pushed the consoleSubcommand branch 2 times, most recently from b6f3a18 to bfb11b8 Compare October 30, 2017 17:02

Console refactor

a955792

RowdyLemon force-pushed the consoleSubcommand branch from bfb11b8 to a955792 Compare October 30, 2017 19:27

ryanmt reviewed Apr 22, 2020

View reviewed changes


		Opens the AWS console in the default web browser. Uses either the credentials in the current environment or the credentials in the specified vault. Console sessions either use the provided vault's duration or defaults to 1 hour.

		Durations must be at least 15 min and less than 12 hours.


		awsKey := c.getAWSKey(vault)

		awsCreds, err := c.getCredentials(vault, awsKey)

Add console subcommand #93

Are you sure you want to change the base?

Add console subcommand #93

Conversation

RowdyLemon commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryan-norton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryan-norton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanmt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment