-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: add flag to disable IMDSv1 fallback #4748
Changes from 9 commits
c30e366
779b3b1
fc88140
1a1b583
4099d5a
7700fc4
edcb42a
1c07b5f
b10ff44
221d436
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package ec2metadata | ||
|
||
import ( | ||
"fmt" | ||
"net/http" | ||
"sync/atomic" | ||
"time" | ||
|
@@ -33,11 +34,15 @@ func newTokenProvider(c *EC2Metadata, duration time.Duration) *tokenProvider { | |
return &tokenProvider{client: c, configuredTTL: duration} | ||
} | ||
|
||
// check if fallback is enabled | ||
func (t *tokenProvider) fallbackEnabled() bool { | ||
return t.client.Config.EC2MetadataEnableFallback == nil || *t.client.Config.EC2MetadataEnableFallback | ||
} | ||
|
||
// fetchTokenHandler fetches token for EC2Metadata service client by default. | ||
func (t *tokenProvider) fetchTokenHandler(r *request.Request) { | ||
|
||
// short-circuits to insecure data flow if tokenProvider is disabled. | ||
if v := atomic.LoadUint32(&t.disabled); v == 1 { | ||
if v := atomic.LoadUint32(&t.disabled); v == 1 && t.fallbackEnabled() { | ||
return | ||
} | ||
|
||
|
@@ -49,23 +54,21 @@ func (t *tokenProvider) fetchTokenHandler(r *request.Request) { | |
output, err := t.client.getToken(r.Context(), t.configuredTTL) | ||
|
||
if err != nil { | ||
// only attempt fallback to insecure data flow if IMDSv1 is enabled | ||
if !t.fallbackEnabled() { | ||
r.Error = awserr.New("EC2MetadataError", "failed to get IMDS token and fallback is disabled", err) | ||
return | ||
} | ||
|
||
// change the disabled flag on token provider to true, | ||
// when error is request timeout error. | ||
// change the disabled flag on token provider to true and fallback | ||
if requestFailureError, ok := err.(awserr.RequestFailure); ok { | ||
switch requestFailureError.StatusCode() { | ||
case http.StatusForbidden, http.StatusNotFound, http.StatusMethodNotAllowed: | ||
atomic.StoreUint32(&t.disabled, 1) | ||
t.client.Config.Logger.Log(fmt.Sprintf("WARN: failed to get session token, falling back to IMDSv1: %v", requestFailureError)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
case http.StatusBadRequest: | ||
r.Error = requestFailureError | ||
} | ||
|
||
// Check if request timed out while waiting for response | ||
if e, ok := requestFailureError.OrigErr().(awserr.Error); ok { | ||
if e.Code() == request.ErrCodeRequestError { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: did we ever get a handle on what was triggering this code path? From when I looked at it, the comment didn't appear accurate, it seemed like anything in the handler stack could have made it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is my concern and maybe a reason that fallback is happening more often than it should. This seems as though it can trigger too easily. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed. the original Standard states that this should be for "timeouts". im wondering if it should be checking for EDIT: this was meant to be a reply to @lucix-aws and @aajtodd thread but github seemed to format it as a separate thread There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We talked offline about this and decided to just remove this. This was triggering permanent fallback for likely far too many error response codes. In the event of a timeout error it will still fallback for the current request but will retry imdsv2 for subsequent requests. |
||
atomic.StoreUint32(&t.disabled, 1) | ||
} | ||
} | ||
} | ||
return | ||
} | ||
|
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.
nit: i think this error message could have specified what version of IMDS failed and what version its falling back to. so like: