Skip to content

Validate that localhost resolves to loopback address #3672

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 17, 2023
Merged

Validate that localhost resolves to loopback address #3672

merged 4 commits into from
Jan 17, 2023

Conversation

dave-fn
Copy link
Contributor

@dave-fn dave-fn commented Jan 5, 2023 •
edited
Loading

Motivation and Context

HTTP(S) credential provider requires the implementation to verify that the resolved addresses for the host are actually loopback addresses.
InetAddress is used to resolve DNS names.

Testing

Added unit tests.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@dave-fn dave-fn requested a review from a team as a code owner January 5, 2023 18:09
millems
millems reviewed Jan 5, 2023
View reviewed changes
Copy link
Contributor

@millems millems left a comment

Choose a reason for hiding this comment

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

Is there a similar change we need to make for IMDS?

.changes/next-release/bugfix-AmazonElasticContainerServiceECS-09f6115.json Outdated
Comment on lines 1 to 6
{
"category": "Amazon Elastic Container Service (ECS)",
"contributor": "",
"type": "bugfix",
"description": "HTTP(S) credential provider requires the implementation to verify that the resolved addresses for the host are actually loopback addresses."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't want a changelog entry for this one, since it's security-related. Otherwise we'd want to talk with security about wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove.

...auth/src/main/java/software/amazon/awssdk/auth/credentials/ContainerCredentialsProvider.java Outdated
Comment on lines 245 to 249
if (inetAddress instanceof Inet6Address) {
return ALLOWED_HOSTS_IPv6_RULES.test(inetAddress);
}

return ALLOWED_HOSTS_IPv4_RULES.test(inetAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are the same predicate, can we simplify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged into a single constant. If later on, there is a need to differentiate between IPv4 and IPv6 we can create separate constants.

...auth/src/main/java/software/amazon/awssdk/auth/credentials/ContainerCredentialsProvider.java

private boolean isAllowedHost(String host) {
try {
InetAddress[] addresses = InetAddress.getAllByName(host);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a comment that this is best-effort: if DNS caching is disabled or the cache expires between our check here and when we actually invoke the API, the endpoint might not be loopback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add Javadoc.

@dave-fn dave-fn force-pushed the davidfn/ops/localhost-resolves-to-loopback branch from f112c47 to 040b4ae Compare January 6, 2023 17:32
@dave-fn
Copy link
Contributor Author

dave-fn commented Jan 6, 2023 •
edited
Loading

Is there a similar change we need to make for IMDS?

It appears IMDS clients can configure the endpoint, and even when not, the default values are 169.254.169.254 and [fd00:ec2::254]

millems
millems approved these changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@millems millems left a comment

Choose a reason for hiding this comment

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

LGTM, added a small non-functional comment/question. Non-blocking.

...auth/src/main/java/software/amazon/awssdk/auth/credentials/ContainerCredentialsProvider.java Outdated
Comment on lines 216 to 217
"%s has an invalid host. Host should resolve to a loopback" +
" address.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also include that making it HTTPS is an alternative way to fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Updated exception message.

@dave-fn dave-fn force-pushed the davidfn/ops/localhost-resolves-to-loopback branch from a52266a to a8711bc Compare January 6, 2023 18:24
@dave-fn dave-fn enabled auto-merge (squash) January 13, 2023 17:46
@dave-fn dave-fn force-pushed the davidfn/ops/localhost-resolves-to-loopback branch 2 times, most recently from f93ee93 to 86fbfc2 Compare January 13, 2023 20:54
@dave-fn dave-fn disabled auto-merge January 13, 2023 23:37
@dave-fn dave-fn enabled auto-merge (squash) January 13, 2023 23:37
@dave-fn dave-fn disabled auto-merge January 13, 2023 23:37
@dave-fn dave-fn enabled auto-merge (squash) January 13, 2023 23:37
@dave-fn dave-fn disabled auto-merge January 13, 2023 23:41
@dave-fn dave-fn force-pushed the davidfn/ops/localhost-resolves-to-loopback branch from d177fd0 to 76df19e Compare January 17, 2023 17:55
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

72.7% 72.7% Coverage
0.0% 0.0% Duplication

@dave-fn dave-fn merged commit 5fc1347 into master Jan 17, 2023
@dave-fn dave-fn deleted the davidfn/ops/localhost-resolves-to-loopback branch January 17, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millems millems millems approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dave-fn @millems