[DO NOT MERGE, WIP]Add support to fetch cacertfiles from Amazon S3 #14572
Conversation
lukebakken
force-pushed
the
aws-resource-fetcher
branch
from
031d6b0 to
e711234
Compare
| lists:foreach(fun({_Key, Arn, Handler}) -> | ||
| case resolve_arn(Arn) of | ||
| {ok, Content} -> | ||
| FilePath = write_to_file(Arn, Content), |
There was a problem hiding this comment.
There is no need to write these files to the filesystem if we use the cacerts configuration option. Please see this discussion for more information:
|
|
||
| %% Export all for unit tests | ||
| -ifdef(TEST). | ||
| -compile(export_all). |
There was a problem hiding this comment.
It would be better to be specific about which functions are exported for tests.
|
|
||
| -spec resolve_arn(string()) -> {ok, binary()} | {error, term()}. | ||
| resolve_arn(Arn) -> | ||
| ?LOG_INFO("Attempting to resolve ARN: ~p", [Arn]), |
There was a problem hiding this comment.
| ?LOG_INFO("Attempting to resolve ARN: ~p", [Arn]), | |
| ?LOG_DEBUG("aws arn: resolving: ~p", [Arn]), |
| end, | ||
| ?LOG_INFO("Fetched ARN ~p and stored to ~p", [Arn, FilePath]); | ||
| {error, Reason} -> | ||
| ?LOG_ERROR("Failed to resolve ARN ~p: ~p", [Arn, Reason]) |
There was a problem hiding this comment.
| ?LOG_ERROR("Failed to resolve ARN ~p: ~p", [Arn, Reason]) | |
| ?LOG_ERROR("aws arn: failed to resolve ~p: ~p", [Arn, Reason]) |
| ldap_ssl_options_cacertfile -> | ||
| update_env(rabbitmq_auth_backend_ldap, ssl_options, cacertfile, FilePath) | ||
| end, | ||
| ?LOG_INFO("Fetched ARN ~p and stored to ~p", [Arn, FilePath]); |
There was a problem hiding this comment.
| ?LOG_INFO("Fetched ARN ~p and stored to ~p", [Arn, FilePath]); | |
| ?LOG_INFO("aws arn: fetched ~p and stored to ~p", [Arn, FilePath]); |
| {ok, #{service := "s3"}} -> | ||
| fetch_s3_object(Arn); | ||
| {ok, #{service := Service}} -> | ||
| ?LOG_ERROR("Unsupported service: ~p", [Service]), |
There was a problem hiding this comment.
| ?LOG_ERROR("Unsupported service: ~p", [Service]), | |
| ?LOG_ERROR("aws arn: unsupported service: ~p", [Service]), |
| ?LOG_ERROR("Unsupported service: ~p", [Service]), | ||
| {error, {unsupported_service, Service}}; | ||
| {error, _} = Error -> | ||
| ?LOG_ERROR("Failed to parse ARN ~p: ~p", [Arn, Error]), |
There was a problem hiding this comment.
| ?LOG_ERROR("Failed to parse ARN ~p: ~p", [Arn, Error]), | |
| ?LOG_ERROR("aws arn: failed to parse ~p: ~p", [Arn, Error]), |
| end. | ||
|
|
||
| -spec parse_arn(string()) -> {ok, map()} | {error, term()}. | ||
| parse_arn(Arn) -> |
There was a problem hiding this comment.
If Arn can be a string(), then it shouldn't be necessary to convert it to a binary and then use binary:split. I recommend string:tokens.
| case binary:split(ArnBin, <<":">>, [global]) of | ||
| [<<"arn">>, Partition, Service, Region, Account, Resource] -> | ||
| {ok, #{ | ||
| partition => binary_to_list(Partition), |
There was a problem hiding this comment.
Why are we doing conversions to lists here?
There was a problem hiding this comment.
+1 I also noted the conversion here. The input from {mapping, "aws.arns.auth_ldap.ssl_options.cacertfile", "rabbit.aws_arns", [{datatype, string}]}. is a binary.Can we just use binary:split ? @sunfinite
There was a problem hiding this comment.
If the reason of using list is because you need to use string:tokens against the output, considering using string:lexemes which takes in either list or binary
|
An alternative to using |
BeichenZhang-BCZ
changed the title
lukebakken
force-pushed
the
aws-resource-fetcher
branch
from
e711234 to
07d22a9
Compare
In collaboration with Sunfinite - thanks for kicking off first commit
Proposed Changes
Please describe the big picture of your changes here to communicate to the RabbitMQ team why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.
When rabbitmq user configures or operates rabbitmq, user may want to configure rabbitmq to access information such as private CA certs/credentials etc, in AWS service rather than passing those information as local file or plain-text configuration to improve security and usability. For example, if user intends to configure LDAP for authentication and authorization, user as of today needs to define a local certs file and inform rabbitmq with config or environment variable .
With this AWS Resource Fetcher feature, user can pass in Amazon Resource Name(ARN) of the certs through new configuration schema eg:
aws.arns.auth_ldap.ssl_options.cacertfile. The fetcher will fetch the relevant cert files from AWS S3 from user's account and assign it to the appropriate LDAP plugin application environment.For corner case handling:
This PR is working in progress to solicit early feedbacks. We are working on to add more functionalities such as allowing customer to configure Secret Manager arn for rabbitmq to retrieve credentials.
Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply.You can also fill these out after creating the PR.
This is simply a reminder of what we are going to look for before merging your code.
CONTRIBUTING.mddocumentFurther Comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution
you did and what alternatives you considered, etc.