out_http: Add AWS SigV4 Authentication Option to out_http #5165
Conversation
matthewfala
requested review from
PettitWesley,
edsiper,
leonardo-albertovich,
fujimotos and
koleini
as code owners
matthewfala
force-pushed
the
http-aws-sigv4
branch
from
a5e40b8 to
9045ca6
Compare
matthewfala
changed the title
matthewfala
changed the title
matthewfala
force-pushed
the
http-aws-sigv4
branch
from
9045ca6 to
d5bd233
Compare
plugins/out_http/http_conf.c
Outdated
| #ifdef FLB_HAVE_SIGNV4 | ||
| #ifdef FLB_HAVE_AWS | ||
| if (ctx->has_aws_auth) { | ||
| ctx->aws_service = flb_output_get_property("aws_service", ctx->ins); |
There was a problem hiding this comment.
Why we cannot set aws_service in FLB_AWS_CREDENTIAL_BASE_CONFIG_MAP as well and put it in lb_managed_chain_provider_create
There was a problem hiding this comment.
the managed_chain_provider_create is meant to be a general credential provider base init for all plugins using credential services. We can easily migrate cloudwatch, firehose, kinesis, s3, and es, to use this base as well since they all share the same 4 output properties.
aws_service is only used for sigv4, and doesn't generalize well to be part of the credential provider.
| } | ||
|
|
||
| /* Use null sts_endpoint if none provided */ | ||
| sts_endpoint = flb_output_get_property(config_key_sts_endpoint, ins); |
There was a problem hiding this comment.
we could add a debug log here say that we use null sts_endpoint as none provided
There was a problem hiding this comment.
Null sts endpoint means that we are not using sts. I don't think a debug statement is needed.
There was a problem hiding this comment.
I don't think this needs a debug statement either. Also, if the custom sts endpoint is NULL we will later construct the regional STS endpoint in the provider itself.
| strcpy(config_key_external_id + key_prefix_len, "external_id"); | ||
|
|
||
| /* AWS provider needs a separate TLS instance */ | ||
| cred_tls = flb_tls_create(FLB_TRUE, |
There was a problem hiding this comment.
nit: do we want to check the format a little bit? more spaces in rest lines
There was a problem hiding this comment.
Yep, sounds good.
src/aws/flb_aws_credentials.c
Outdated
|
|
||
| /* STS provider needs yet another separate TLS instance */ | ||
| sts_tls = flb_tls_create(FLB_TRUE, | ||
| ins->tls_debug, |
| { \ | ||
| FLB_CONFIG_MAP_STR, prefix "role_arn", NULL, \ | ||
| 0, FLB_FALSE, 0, \ | ||
| "ARN of an IAM role to assume (ex. for cross account access)" \ |
There was a problem hiding this comment.
Change to AWS IAM Role so align with line 186 to reduce confusion.
There was a problem hiding this comment.
Aligning with value in Cloudwatch, Firehose, Kinesis, and S3. Future plans to potentially unify the code for credential provider setup, need to keep this
ARN of an IAM role to assume
| if (ctx->has_aws_auth == FLB_TRUE) { | ||
| flb_plg_debug(ctx->ins, "signing request with AWS Sigv4"); | ||
| signature = flb_signv4_do(c, | ||
| FLB_TRUE, /* normalize URI ? */ |
There was a problem hiding this comment.
what are the question mark for?
| flb_plg_debug(ctx->ins, "signing request with AWS Sigv4"); | ||
| signature = flb_signv4_do(c, | ||
| FLB_TRUE, /* normalize URI ? */ | ||
| FLB_TRUE, /* add x-amz-date header ? */ |
There was a problem hiding this comment.
Booleans. Actually copied this comment from other plugins
| } | ||
|
|
||
| /* initialize credentials in sync mode */ | ||
| aws_provider->provider_vtable->sync(aws_provider); |
There was a problem hiding this comment.
could you explain why it need to be set as sync at first and set back to async quickly?
There was a problem hiding this comment.
The way i understand it is that during plugin init, there is no such thing as async networking (this might have changed recently). That is because setup occurs before the coroutines are allowed to swap out. Really the reason why I did this was because everywhere else we setup the cred provider, we do this.
There was a problem hiding this comment.
Yea I added this a while ago to fix a bug... AFAIK the init callbacks still must run in sync mode.
| ins->tls_key_file, | ||
| ins->tls_key_passwd); | ||
| if (!sts_tls) { | ||
| flb_errno(); |
There was a problem hiding this comment.
Could we add an error log for this error too?
| ins->tls_key_file, | ||
| ins->tls_key_passwd); | ||
| if (!cred_tls) { | ||
| flb_errno(); |
There was a problem hiding this comment.
Could we add an error log for this error too?
plugins/out_http/http.c
Outdated
| 0, FLB_TRUE, offsetof(struct flb_out_http, aws_service), | ||
| "AWS destination service code, used by SigV4 authentication" | ||
| }, | ||
| FLB_AWS_CREDENTIAL_BASE_CONFIG_MAP("aws_"), |
There was a problem hiding this comment.
I like the idea to make the config general to help on reusing it.
The aws_ value looks to me like a static value and has been used several different place below.
Could we create a static field somewhere and refer to the same place so in case in the future this name is not suitable we don't need to search everywhere to change it.
There was a problem hiding this comment.
Actually this is the only place it is used. Regardless, I'll make it a #define
It's used multiple times actually.
plugins/out_http/http_conf.c
Outdated
| ctx->aws_provider = flb_managed_chain_provider_create( | ||
| ins, | ||
| config, | ||
| "aws_", |
There was a problem hiding this comment.
Make the aws_ refer to a static field
matthewfala
force-pushed
the
http-aws-sigv4
branch
from
d5bd233 to
aaa58c1
Compare
|
Resolved all comments. Would you take another look @DrewZhang13 and @zhonghui12? |
src/aws/flb_aws_credentials.c
Outdated
|
|
||
| session_name = flb_sts_session_name(); | ||
| if (!session_name) { | ||
| flb_plg_error(ins, "Failed to create aws iam role " |
There was a problem hiding this comment.
nit: use the word generate not create to help make it clear this was our code failing vs some create call to STS
| /* Provider managed dependencies; to delete on destroy */ | ||
| struct flb_aws_provider *base_aws_provider; | ||
| struct flb_tls *cred_tls; /* tls instances can't be re-used; aws provider requires | ||
| a separate one */ | ||
| struct flb_tls *sts_tls; /* one for the standard chain provider, one for sts | ||
| assume role */ |
There was a problem hiding this comment.
Hmmm this is only used by the new flb_managed_chain_provider_create function to use? the other providers don't use these new fields I guess?
There was a problem hiding this comment.
That's right. It's just if we want aws_provider_destroy to handle clean up operations. If null (due to calloc) clean up will not occur
Signed-off-by: Matthew Fala <[email protected]>
Signed-off-by: Matthew Fala <[email protected]>
matthewfala
dismissed stale reviews from PettitWesley and DrewZhang13
via
f51496c
matthewfala
force-pushed
the
http-aws-sigv4
branch
from
aaa58c1 to
f51496c
Compare
Documentation
Documentation PR in review.
See: fluent/fluent-bit-docs#760
SigV4 Results with Valgrind
Config File
Valgrind output
Result Headers (from http server)
Valgrind SigV4 Error Test
Config File
Valgrind Output
One Click Replication Configuration
via Firelens Datajet
HTTP loopback validator featured only on the
http-validatorbranchHere's a link to the firelens-datajet project which aims to make replicating fluent bit end to end testing portable.
https://github.com/aws/firelens-datajet/tree/http-validator
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.