Add payload hash func and default host var; Move fetching credentials func to sig lib #126

shawnhankim · 2023-05-06T17:58:19Z

To enhance common AWS sig & credentials lib for services in AWS.

Description:

Add a payload hash function to support multiple services in AWS such as S3 and Lambda, and it can be used for not only GET but also POST method.
Add a NJS var of default host name to be used for Role Session Name at multiple services in AWS
Move reusable functions of fetching credentials into awscredentials.js
Replace name of env variables of AWS credentials and debug to support common AWS signature lib for multiple services in AWS

Misc.:

…edentials and debug - Move reusable functions of fectching credentials into awscredentials.js - Replace name of env variables of AWS credentials and debug to support common AWS signature lib for multiple services in AWS - Add a NJS var of default host name to be used for Role Session Name at multiple services in AWS - Add a payload hash function to support multiple services in AWS such as S3 and Lambda, and it can be used for not only GET but also POST method.

dekobon

Hi @shawnhankim thank you for the solid contribution. My apologies for the delay on the review. This all looks good and I had only a few comments/suggestions.

dekobon · 2023-05-09T16:03:20Z

common/etc/nginx/include/awscredentials.js

@@ -34,24 +68,26 @@ function sessionToken(r) {
 }

 /**
- * Get the instance profile credentials needed to authenticated against S3 from
+ * Get the instance profile credentials needed to authenticate against S3 from
 * a backend cache. If the credentials cannot be found, then return undefined.
 * @param r {Request} HTTP request object (not used, but required for NGINX configuration)
 * @returns {undefined|{accessKeyId: (string), secretAccessKey: (string), sessionToken: (string|null), expiration: (string|null)}} AWS instance profile credentials or undefined
 */
 function readCredentials(r) {
    // TODO: Change the generic constants naming for multiple AWS services.


This TODO comment can be removed because this PR does what is mentioned.

Thanks for finding this. Addressed!

dekobon · 2023-05-09T16:06:13Z

common/docker-entrypoint.d/00-check-for-required-env.sh

@@ -34,7 +34,7 @@ required=("S3_BUCKET_NAME" "S3_SERVER" "S3_SERVER_PORT" "S3_SERVER_PROTO"
 if [[ -v AWS_CONTAINER_CREDENTIALS_RELATIVE_URI ]]; then
  echo "Running inside an ECS task, using container credentials"



Add a section to this script that detects the presence of S3_ environment variables, emits warnings that they are deprecated, and assigns those environment variables to the appropriate AWS_ environment variables.

It is a great point. Thanks! Addressed. Let me know if you have any additional comments.

dekobon · 2023-05-10T18:48:04Z

common/etc/nginx/templates/default.conf.template

@@ -23,6 +24,7 @@ js_var $indexIsEmpty true;
 js_set $s3auth s3gateway.s3auth;
 js_set $awsSessionToken awscredentials.sessionToken;
 js_set $s3uri s3gateway.s3uri;
+js_var $defaultHostName 'nginx-s3-gateway';


What's the purpose of setting this variable?

This is the purpose of option 2 in this link: #126 (comment)

I have removed this variable, and you could find my answer in the #126 (comment).

dekobon · 2023-05-10T18:48:48Z

common/etc/nginx/include/s3gateway.js

- */
-async function _fetchWebIdentityCredentials(r) {
-    const arn = process.env['AWS_ROLE_ARN'];
-    const name = process.env['HOSTNAME'] || 'nginx-s3-gateway';


How is the hardcoded value nginx-s3-gateway being used? It would be useful to have a comment about it.

This is the existing hardcoded value in this repo's codebase. Looks like it is used as a default Role Session Name (nginx-s3-gateway) in case HOSTNAME is missed.

https://github.com/nginxinc/nginx-s3-gateway/blob/master/docs/getting_started.md?plain=1#L54

As we are moving this function into the common library(awscredentials.js), we can consider one of the following options.

option 1. remove this default value (nginx-s3-gateway) so customers must define the env var (HOSTNAME).

option 2. replace this value with other variable (e.g. $defaultHostName).

option 3. any other suggestion?

FYI:

https://github.com/nginxinc/nginx-s3-gateway/pull/126/files#r1190281902

https://github.com/shawnhankim/nginx-s3-gateway/blob/awssig-env/common/etc/nginx/include/awscredentials.js#L373

https://github.com/shawnhankim/nginx-s3-gateway/blob/awssig-env/common/etc/nginx/templates/default.conf.template#L27

I was wondering what would be your suggestion.

I have removed this hardcoded value in this NJS code, because this NJS can be used for not only S3 but also other services such as Lambda.

I have set this default value in the https://github.com/shawnhankim/nginx-s3-gateway/blob/awssig-env/common/docker-entrypoint.d/00-check-for-required-env.sh#L58 as the following example instead:
elif [[ -v AWS_WEB_IDENTITY_TOKEN_FILE ]]; then echo "Running inside EKS with IAM roles for service accounts" if [[ ! -v HOSTNAME ]]; then # This environment value is used for Role Session Name. The default value is # set as a nginx-s3-gateway unless the value is defined. HOSTNAME="nginx-s3-gateway" fi

Let me know if you have other suggestion, or if you agree with this approach.

This approach is I believe is the best we can do. I was considering modifying the startup scripts such that if the environment variable HOSTNAME is not set that script would run hostname and then set HOSTNAME to the value returned.

That said, I'm a bit confused about the above comment that indicates that HOSTNAME maps to the "Role Session Name" because both of those concepts do not seem the same.

Thanks for your opinion and original intent.

Yeah, I was also confused about the comment and variable. So I have replaced HOSTNAME with AWS_ROLE_SESSION_NAME and added the depreciation message of the variable of HOSTNAME.

- Set nginx-s3-gateway as a default value when HOSTNAME isn't set if AWS_WEB_IDENTITY_TOKEN_FILE is defined - Change depreciation message and terminate the bootstrap when credentials and debug variables are set with the prefix of 'S3_' fix: comment

shawnhankim self-assigned this May 6, 2023

shawnhankim requested a review from dekobon May 6, 2023 18:05

shawnhankim mentioned this pull request May 10, 2023

[WIP] feat: nginx-aws-signature lib submodule #124

Open

dekobon approved these changes May 10, 2023

View reviewed changes

shawnhankim added 2 commits May 10, 2023 21:30

feat: depreciated env variables

c508c3f

shawnhankim force-pushed the awssig-env branch from 40cde00 to b7cd5e2 Compare May 13, 2023 16:49

shawnhankim mentioned this pull request May 13, 2023

chore: replace HOSTNAME with AWS_ROLE_SESSION_NAME, change now func and comment nginxinc/nginx-aws-signature#26

Merged

4 tasks

chore: replace HOSTNAME with AWS_ROLE_SESSION_NAME

9def5ed

dekobon merged commit 449612b into nginx:master May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add payload hash func and default host var; Move fetching credentials func to sig lib #126

Add payload hash func and default host var; Move fetching credentials func to sig lib #126

shawnhankim commented May 6, 2023

dekobon left a comment

dekobon May 9, 2023

shawnhankim May 11, 2023

dekobon May 9, 2023

shawnhankim May 11, 2023

dekobon May 10, 2023

shawnhankim May 11, 2023

shawnhankim May 13, 2023

dekobon May 10, 2023

shawnhankim May 11, 2023

shawnhankim May 13, 2023

dekobon May 16, 2023 •

edited

Loading

shawnhankim May 17, 2023

		@@ -34,7 +34,7 @@ required=("S3_BUCKET_NAME" "S3_SERVER" "S3_SERVER_PORT" "S3_SERVER_PROTO"
		if [[ -v AWS_CONTAINER_CREDENTIALS_RELATIVE_URI ]]; then
		echo "Running inside an ECS task, using container credentials"

Add payload hash func and default host var; Move fetching credentials func to sig lib #126

Add payload hash func and default host var; Move fetching credentials func to sig lib #126

Conversation

shawnhankim commented May 6, 2023

dekobon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekobon May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekobon May 16, 2023 •

edited

Loading