Skip to content

Commit

Permalink
Refactor invalid session duration handling
Browse files Browse the repository at this point in the history
  • Loading branch information
johananl committed Nov 20, 2018
1 parent 2d1816b commit a5c9164
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 42 deletions.
53 changes: 29 additions & 24 deletions aws/sts.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,46 @@ package aws

import (
"errors"
"fmt"
"strings"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/sts"
)

var ErrInvalidSessionDuration = errors.New("InvalidSessionDuration")
const (
// The error message STS returns when attempting to assume a role with a duration longer than
// the configured maximum for that role.
ErrInvalidSessionDuration = "The requested DurationSeconds exceeds the MaxSessionDuration set for this role."
// A custom error which indicates that the requested duration exceeded the configured maximum.
// TODO Replace this with a custom error type.
ErrDurationExceeded = "DurationExceeded"
)

// AssumeSAMLRole asumes a Role using the SAMLAssertion specified. If the duration cannot be meet
// it transperently lowers the duration to the minimal valid value of 3600 seconds and returns an
// error in parallel to the valid credentials.
// AssumeSAMLRole assumes an AWS IAM role using a SAML assertion.
// In cases where the requested session duration is higher than the maximum allowed on AWS, STS
// returns a specific error message to indicate that. In this case we return a custom error to the
// caller to allow special handling such as retrying with a lower duration.
func AssumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion string, duration int64) (*Credentials, error) {
creds, err := assumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion, duration, false)
if err == ErrInvalidSessionDuration {
// the requested duration was invalid. So try again with a minimum of 3600s and return an
// EErrInvalidSessionDuration error, too.
return assumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion, 3600, true)
creds, err := assumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion, duration)
if err != nil {
// Verify error is an AWS error.
if awsErr, ok := err.(awserr.Error); ok {
// Check if error indicates exceeded duration.
if awsErr.Message() == ErrInvalidSessionDuration {
// Return a custom error to allow the caller to retry etc.
// TODO Return a custom error type instead of a special value:
// https://dave.cheney.net/2014/12/24/inspecting-errors
return nil, errors.New(ErrDurationExceeded)
}
}
return nil, err
}

return creds, nil
}

func assumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion string, duration int64, roleDoesNotSupportDuration bool) (*Credentials, error) {
// Assume role
func assumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion string, duration int64) (*Credentials, error) {
input := sts.AssumeRoleWithSAMLInput{
PrincipalArn: aws.String(PrincipalArn),
RoleArn: aws.String(RoleArn),
Expand All @@ -40,12 +54,7 @@ func assumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion string, duration int64,

aResp, err := svc.AssumeRoleWithSAML(&input)
if err != nil {
// The role might not yet support the requested duration, let's catch and return a specific
// error. There is - as of now - no other way than to do a string comparison.
if strings.HasPrefix(err.Error(), "ValidationError: The requested DurationSeconds exceeds the MaxSessionDuration set for this role") && duration > 3600 && duration <= 43200 {
return nil, ErrInvalidSessionDuration
}
return nil, fmt.Errorf("assuming role: %v", err)
return nil, err
}

keyID := *aResp.Credentials.AccessKeyId
Expand All @@ -60,9 +69,5 @@ func assumeSAMLRole(PrincipalArn, RoleArn, SAMLAssertion string, duration int64,
Expiration: expiration,
}

if roleDoesNotSupportDuration {
err = ErrInvalidSessionDuration
}

return &creds, err
return &creds, nil
}
18 changes: 9 additions & 9 deletions okta/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ package okta

import (
"fmt"
"strings"
"log"

"github.com/allcloud-io/clisso/aws"
"github.com/allcloud-io/clisso/config"
"github.com/allcloud-io/clisso/saml"
"github.com/allcloud-io/clisso/spinner"
"github.com/fatih/color"
"github.com/howeyc/gopass"
)

Expand Down Expand Up @@ -104,14 +105,13 @@ func Get(app, provider string, duration int64) (*aws.Credentials, error) {
creds, err := aws.AssumeSAMLRole(arn.Provider, arn.Role, *samlAssertion, duration)
s.Stop()

// the default duration might be shorter than what is configured on AWS side. The code above
// selected the minimum duration. If more was requested print an info.
if err == aws.ErrInvalidSessionDuration {
fmt.Printf("The role does not support the requested duration of %v. To have a max session "+
"duration for up to 12h run:\n", duration)
fmt.Printf("aws iam update-role --role-name %v --max-session-duration 43200 --profile %v\n",
arn.Role[strings.LastIndex(arn.Role, "/")+1:], app)
err = nil
if err != nil {
if err.Error() == aws.ErrDurationExceeded {
log.Println(color.YellowString("Requested duration exceeded allowed maximum. Falling back to 1 hour."))
s.Start()
creds, err = aws.AssumeSAMLRole(arn.Provider, arn.Role, *samlAssertion, 3600)
s.Stop()
}
}

return creds, err
Expand Down
18 changes: 9 additions & 9 deletions onelogin/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ package onelogin

import (
"fmt"
"strings"
"log"
"time"

"github.com/allcloud-io/clisso/aws"
"github.com/allcloud-io/clisso/config"
"github.com/allcloud-io/clisso/saml"
"github.com/allcloud-io/clisso/spinner"
"github.com/fatih/color"
"github.com/howeyc/gopass"
)

Expand Down Expand Up @@ -187,14 +188,13 @@ func Get(app, provider string, duration int64) (*aws.Credentials, error) {
creds, err := aws.AssumeSAMLRole(arn.Provider, arn.Role, rMfa.Data, duration)
s.Stop()

// the default duration might be shorter than what is configured on AWS side. The code above
// selected the minimum duration. If more was requested print an info.
if err == aws.ErrInvalidSessionDuration {
fmt.Printf("The role does not support the requested duration of %v. To have a max session "+
"duration for up to 12h run:\n", duration)
fmt.Printf("aws iam update-role --role-name %v --max-session-duration 43200 --profile %v\n",
arn.Role[strings.LastIndex(arn.Role, "/")+1:], app)
err = nil
if err != nil {
if err.Error() == aws.ErrDurationExceeded {
log.Println(color.YellowString("Requested duration exceeded allowed maximum. Falling back to 1 hour."))
s.Start()
creds, err = aws.AssumeSAMLRole(arn.Provider, arn.Role, rMfa.Data, 3600)
s.Stop()
}
}

return creds, err
Expand Down

3 comments on commit a5c9164

@johananl
Copy link
Collaborator Author

@johananl johananl commented on a5c9164 Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason you totally dropped the awscli line?

@allcloud-jonathan You mean aws sts ... in the warning message we are printing? No, we can add it. This commit isn't the final state, I was concentrating on the error handling logic and on moving the pieces of code to the right place. It can definitely be useful to print a command and/or a link to the docs.

Personally I'd prefer the solution, where I only have to handle the AssumeSAMLRole once, but I could live with this.

What do you mean? Are you talking about the code duplication between the packages onelogin and okta? If so, this is a separate issue outside the scope of this PR, i.e. making the provider part of Clisso generic using Go interfaces so that the calling code doesn't care about the provider type. We actually have an open issue for this.

I moved the "fallback to 1h" handling into the provider packages since it makes more sense to have it there. This makes the error handling in the aws package simple and generic and lets the caller decide if and how they want to retry stuff. It could even make more sense to move this logic to the cmd package but this would require a large-scale refactoring which I'd like to avoid. In any case this is out of scope for this PR.

@johananl
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this logic makes sense to you I can add this commit to the PR. We should review the docs and comments again to make sure they match the latest code and then we should be able to merge.

@johananl
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I'll do my best to complete this tonight (currently at work...). I'll comment on the PR when done.

Please sign in to comment.