Use AWS SDK to retrieve ec2 instance identity doc #1360
Conversation
mcpherrinm
requested review from
amartinezfayo,
APTy,
azdagron,
evan2645 and
marcosy
as code owners
|
force-pushed to fix missing DCO |
| return nil, err | ||
| sigSuffix := "/dynamic/" + sigPath | ||
| if !strings.HasSuffix(sigURL, sigSuffix) { | ||
| return "", fmt.Errorf("IID signature URL '%s' doesn't end in expected suffix %s", sigURL, sigSuffix) |
There was a problem hiding this comment.
This is source #1 of incompatibility: If you don't use the regular path suffix, it's not going to work.
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode) | ||
| if docEndpoint != sigEndpoint { | ||
| return "", fmt.Errorf("IID URL and Signature URL had different endpoints: %s != %s", docEndpoint, sigEndpoint) |
There was a problem hiding this comment.
This is source #2 of incompatibility: If you use two different endpoints it doesn't work -- the SDK only takes a single one.
|
I moved some of the changes to a separate PR to make this easier to review: |
This allows use of the v2 instance identity metadata service, which has improved resilience to SSRF and similar vulnerabilities. There is some backwards compatibility with the previous configuration that specifies full URLs, however, the SDK wants you to configure a single endpoint. This change is fully backwards compatible when using AWS IID or a direct proxy for it. It is theoretically incompatible if you used nonstandard URLs, but that is unlikely given the response must be signed with a fixed Amazon controlled private key. Signed-off-by: Matthew McPherrin <mmc@squareup.com>
|
Rebased on master now that #1362 is merged |
|
I'm also going to open an alternative PR as well that keeps both codepaths side by side for the deprecation period, and see how that looks in comparison. |
|
We should strive to maintain back-compat with a deprecation warning. That being said, as you've pointed out, it doesn't seem likely that there is working configuration out in the wild that this change would break. How far are you on the alternative PR? If that one is ergonomic and easy to rip out after the deprecation period, that would would probably be preferred, but if it is looking greasy, I'd probably be ok with this PR as-is. |
|
I haven't had a chance to work on an alternative PR this week. This PR still needs deprecation warnings and documentation updates |
That sounds great, thanks @mcpherrinm and apologies for the trouble. |
|
Declining in favor if #1369 which seems better |
Pull Request check list
Affected functionality
EC2 IID node attestation
Description of change
Use AWS SDK to retrieve ec2 instance identity doc
This allows use of the v2 instance identity metadata service, which has improved resilience to SSRF and similar vulnerabilities.
There is some backwards compatibility with the previous configuration that specifies full URLs, however, the SDK wants you to configure a single endpoint.
This change is fully backwards compatible when using AWS IID or a direct proxy for it. It is theoretically incompatible if you used nonstandard URLs, but that is unlikely given the response must be signed with a fixed Amazon controlled private key.
Still to resolve:
Is this sufficient backwards compatibility? I think to fully support the old configuration, we'd need two codepaths. Is that worth the burden of a relatively-unlikely configuration? I haven't yet added any deprecation warnings; should I simply log a warn-level message?
Which issue this PR fixes
fixes #1359