Skip to content

Support rabbit_peer_discovery_aws to work with EC2 IMDSv2 #2956

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

Merged
merged 13 commits into from
Apr 8, 2021
Merged

Support rabbit_peer_discovery_aws to work with EC2 IMDSv2 #2956

merged 13 commits into from
Apr 8, 2021

Conversation

thuandb
Copy link
Contributor

@thuandb thuandb commented Apr 7, 2021

Hi, I’m from the Amazon MQ team in AWS. We are proposing the following change to prefer using the EC2 Instance Metadata Service v2 based on AWS security recommendations.

Proposed Changes

The following change is an implementation to support the rabbit_peer_discovery_aws plugin to work with EC2 instance metadata service version 2 (IMDSv2). IMDSv2 is a new version of the instance metadata service which adds defense in depth against open firewalls, reverse proxies, and SSRF vulnerabilities. With IMDSv2, every request submitted to the instance metadata service is now protected by session authentication. Specifically, querying IMDSv2 is done in 2 steps:

  • Start a session with an HTTP PUT request to IMDSv2. IMDSv2 then returns a secret token.
  • Use the secret token to make any HTTP GET request to retrieve interested metadata values such as availability zone, instance role, instance ID, credentials, etc.

Both IMDSv1 and IMDSv2 are available by default on all EC2 instances. This change is backward compatible. If for some reason, it fails to obtain a secret token from IMDSv2, rabbit_peer_discovery_aws plugin will automatically fallback to use IMDSv1.

Per AWS security recommendation, IMDSv2 is preferable. A new configuration flag aws.prefer_imdsv2 is introduced to prefer using IMDSv2. Specifically, if the configuration flag aws.prefer_imdsv2 is set to true (which is also the default value), the rabbitmq_peer_discovery_aws plugin will attempt to use IMDSv2 by retrieving a secret token. If a secret token is received successfully, it will be used in subsequent requests submitted to the Instance Metadata Service. If a secret token cannot be obtained, then subsequent calls will be made to IMDSv1. On the other hand, if aws.prefer_imdsv2 is set to false, the rabbitmq_peer_discovery_aws plugin will only use IMDSv1 for instance metadata queries.

Notes

IMDSv2 secret token retrieval result (both success and failure) is cached and reused. This is to guarantee that there is at most 1 HTTP PUT request is used to request IMDSv2 secret token per session which included multiple HTTP GET requests to query metadata values: instance-id, availability zone, instance role, and credentials.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have tested this change on AWS.
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Copy link
Collaborator

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

We generally don't reject pull requests over things like this but in this PR, multiple lines
do not use any whitespace when binding or destructuring:

{ok, State}=gen_server:call(rabbitmq_aws, get_state)

%% When false, EC2 IMDSv1 will be used first and no attempt will be made to EC2 IMDSv2..
%% See https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-instance-metadata-service.html.

{mapping, "aws.prefer_imdsv2", "rabbit.aws_prefer_imdsv2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This arguably belongs to rabbitmq_aws which can have its own Cuttlefish schema file. Nothing in RabbitMQ uses AWS or is aware of it.

Copy link
Collaborator

@michaelklishin michaelklishin Apr 7, 2021

Choose a reason for hiding this comment

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

I should perhaps clarify that RabbitMQ will discover all Cuttlefish schema files under priv/schema in every plugin on the code path, so I can't think of a reason why this can't be moved to rabbitmq_aws.

See rabbit_prelaunch_conf if you are curious about how schema file discovery is performed. TL;DR: we simply scan priv/schema directories of all applications on the code path :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestion. I was afraid of having to add a new schema parser to rabbitmq_aws hence tried to reuse rabbit.schema. However, per your feedback, I've moved this AWS specific configuration flag to a new filerabbitmq_aws.schema in the rabbitmq_aws/priv/schema folder.

%% end
get_instruction_on_instance_metadata_error(ErrorMessage) ->
ErrorMessage ++
" Please refer to the AWS documentation for details on how to configure the instance metadata service: "
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good idea to log a link to a doc guide 👍

@thuandb thuandb requested a review from michaelklishin April 8, 2021 00:10
@michaelklishin michaelklishin merged commit 51666bd into rabbitmq:master Apr 8, 2021
@michaelklishin
Copy link
Collaborator

Follow-up PR: #2959.

@michaelklishin
Copy link
Collaborator

Backported to v3.8.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants