Add authentication support for session tokens to AWS. by DennisHeimbigner · Pull Request #3244 · Unidata/netcdf-c

DennisHeimbigner · 2026-01-06T20:57:23Z

Add authentication support for session tokens to AWS.

re: Issue #3136
replaces: https://github.com/alexandervladsemenov/netcdf-c/tree/temp_credentials

Primary change: Session Token Support

As noted in Issue #3136, The goal is to support session tokens used in the HDF5 ROS3 driver.

WARNING: this PR compiles and builds but the basic token functionality as not been tested because we have no access to a server that requires session tokens.

This requires the following modifications:

Add a session_token field to NCglobalstate.aws.
Define constants for session token:
- AWS_RC_SESSION_TOKEN -- session token as .ncrc key
- AWS_PROF_SESSION_TOKEN -- session token as aws profile entry
- AWS_ENV_SESSION_TOKEN -- session token as env variable
- AWS_FRAG_SESSION_TOKEN -- session token as URL fragment key
Get session_token value from (in order) URL fragment, env variable, aws profile, .ncrc entry.
Modify the signature of a number of S3 related functions to include a session_token argument.
Setup to pass the session token to the HDF5 ros3 driver.

# re: Issue Unidata#3136 # replaces: https://github.com/alexandervladsemenov/netcdf-c/tree/temp_credentials ## Primary change: Session Token Support As noted in [Issue Unidata#3136](Unidata#3136), The goal is to support session tokens used in the HDF5 ROS3 driver. WARNING: this PR compiles and builds but the basic token functionality as not been tested because we have no access to a server that requires session tokens. This requires the following modifications: * Add a session_token field to *NCglobalstate.aws*. * Define constants for session token: - AWS_RC_SESSION_TOKEN -- session token as .ncrc key - AWS_PROF_SESSION_TOKEN -- session token as aws profile entry - AWS_ENV_SESSION_TOKEN -- session token as env variable - AWS_FRAG_SESSION_TOKEN -- session token as URL fragment key * Get session_token value from (in order) URL fragment, env variable, aws profile, .ncrc entry. * Modify the signature of a number of S3 related functions to include a session_token argument. * Setup to pass the session token to the HDF5 ros3 driver.

mannreis

I did a quick unsolicited review and nothing major popped out. I still left a couple of comments.

I manually tested this PR for regressions on authenticated S3 read operations on both netcdf4 and zarr data. No issue found.

Notes: on the AWS topic but not for this PR

I don't like "no" being used as a sentinel for aws.profile

Credential loading precedence behaviour should be reviewed (#3162)

mannreis · 2026-01-13T14:04:29Z

RELEASE_NOTES.md

+* Add authentication support for session tokens for HDF5 ROS3
+driver. WARNING: this PR compiles and builds but the basic token
+functionality has not been tested because we have no access to a
+server that requires session tokens. See [Github????](https://github.com/Unidata/netcdf-c/issues/????) for more information.


Suggested change

server that requires session tokens. See [Github????](https://github.com/Unidata/netcdf-c/issues/????) for more information.

server that requires session tokens. See [Github #3136](https://github.com/Unidata/netcdf-c/issues/3136) for more information.

mannreis · 2026-01-13T14:41:31Z

libdispatch/ds3util.c

+
+#if 0
 /**
 * Get the credentials for a given profile or load them from environment.
 @param profile name to use to look for credentials
 @param region return region from profile or env
 @param accessid return accessid from progile or env
 @param accesskey return accesskey from profile or env
 */
-void NC_s3getcredentials(const char *profile, const char **region, const char** accessid, const char** accesskey) {
+void
+NC_s3getcredentials(const char *profile, const char **region, const char** accessid, const char** accesskey, const char** session_token)
+{
    if(profile != NULL && strcmp(profile,"no") != 0) {
        NC_s3profilelookup(profile, AWS_PROF_ACCESS_KEY_ID, accessid);
        NC_s3profilelookup(profile, AWS_PROF_SECRET_ACCESS_KEY, accesskey);
        NC_s3profilelookup(profile, AWS_PROF_REGION, region);
+        NC_s3profilelookup(profile, AWS_PROF_SESSION_TOKEN, session_token);
    }
    else
    { // We load from env if not in profile
        NCglobalstate* gstate = NC_getglobalstate();
-        if(gstate->aws.access_key_id != NULL && accessid){
-            *accessid = gstate->aws.access_key_id;
+        if(gstate->aws->access_key_id != NULL && accessid){
+            *accessid = gstate->aws->access_key_id;
        }
-        if (gstate->aws.secret_access_key != NULL && accesskey){
-            *accesskey = gstate->aws.secret_access_key;
+        if (gstate->aws->secret_access_key != NULL && accesskey){
+            *accesskey = gstate->aws->secret_access_key;
        }
-        if(gstate->aws.default_region != NULL && region){
-            *region = gstate->aws.default_region;
+        if(gstate->aws->default_region != NULL && region){
+            *region = gstate->aws->default_region;
        }
    }
 }
-
+#endif


This helper function was introduced by me in #3098. Perhaps not the best approach.

The goal was to make that the AWS environment variables take precedence and overwrite was possibly loaded from the config file(s). Behaving much like other S3 tools.

However the logic was removed from the code path by #3159

I noted it on the comment:

It is not a problem for me to have the current logic but we are left with dead code which is annoying! So I suggest remove it. Also the declaration!

mannreis · 2026-01-13T14:46:26Z

libdispatch/nch5s3comms.c

+    if(getenv("CURLOPT_VERBOSE") != NULL) {
+	if (CURLE_OK != curl_easy_setopt(curlh, CURLOPT_VERBOSE, 1L))
+            HGOTO_ERROR(H5E_ARGS, NC_EINVAL, NULL, "error while setting CURL option (CURLOPT_VERBOSE).");
+    }
+


Is this indented to be done? I like the fact that we can enable CURL traces!

WardF · 2026-01-13T18:21:51Z

I'm also reviewing and it looks good in general, what remains to remove it from 'Draft' status?

DennisHeimbigner · 2026-01-13T18:32:46Z

The draft was to prevent it from merging before 3229.
so I will remove draft status.

Manuel had some comments:

I don't like "no" being used as a sentinel for aws.profile

What would you propose?

Credential loading precedence behaviour should be reviewed (#3162)

I think this is well defined in the code. Is there a specific problem?

DennisHeimbigner · 2026-01-13T19:00:48Z

There is some confusion here. The two comments above for ds3util.c andd nch5s3comms.c
are already in the code. Did someone already change these, or is Manuel looking at old code?

mannreis

What would you propose?

I'll create an issue to track that, but the first idea is to use aws.profile=NULL to represent profile not selected.

Credential loading precedence behaviour should be reviewed (#3162)

I think this is well defined in the code. Is there a specific problem?

I'll update the issue but I found something on this PR that shows that the environment loading is brittle!

Try to run:

export AWS_PROFILE=''; export AWS_ACCESS_KEY_ID
=""; export AWS_SECRET_ACCESS_KEY=""; ./ncdump/ncdump -L10 -h 'https://s3.us-west-2.amazonaws.com/ncar-cesm-lens/atm/daily/cesmLE
-20C-FLNS.zarr#mode=zarr,s3'

I get a SIGSEGV

We are not testing environment loading extensively. I've signaled the code that causes this in comments but you can see this patch: aeb6044

There is some confusion here.The two comments above for ds3util.c andd nch5s3comms.c
are already in the code. Did someone already change these, or is Manuel looking at old code?

I don't understand the confusion. I reviewed the PR in the github. webUI and I can't touch your fork, but Ward pushed merge commit that might be the source of confusion. However I believe the PR points at the most up-to-date version of s3auth.dmh which will contain that merge commit!

My 2 comments were regarding:

Suggesting deleting NC_s3getcredentials(const char *profile, .... since it's dead code
Checking if it is intended that setting CURLOPT_VERBOSE in the environment directly controls traces. I personally don't mind!

Feel free to close them and move on.

PS: I'm sorry for the long comment.

mannreis · 2026-01-14T09:49:53Z

libdispatch/ds3util.c

 NC_aws_load_credentials(NCglobalstate* gstate)
 {
    int stat = NC_NOERR;
+    size_t i;


Suggested change

size_t i;

int i;

Later, on line 576 i is used as signed int

for(i=nclistlength(profiles)-1;i>=0;i--) {/* walk backward because we are removing entries */

mannreis · 2026-01-14T09:59:03Z

libdispatch/ds3util.c

+		if((entry = (struct AWSentry*)calloc(1,sizeof(struct AWSentry)))==NULL) {stat = NC_ENOMEM; goto done;}
+		entry->key = strdup(AWS_PROF_SESSION_TOKEN);
+		entry->value = strdup(gs->aws->session_token);
+		nclistpush(dfalt->entries,entry); entry = NULL;


This can be a SEGFAULT. dfalt was set to NULL in line 587:

nclistpush(profiles,dfalt); dfalt = NULL;

I suggest either removing dfalt=NULL or moving that statement down

WardF · 2026-01-14T17:41:16Z

Thanks @mannreis, environment loading can be pretty brittle, we need to do more to expand our testing there as well.

I'm currently fixing up my Windows dev environment, it has grown a bit stale; that should take a day or two but it will clear a roadblock that will help me refocus on other efforts.

mannreis · 2026-01-15T17:13:54Z

With that in mind I opened a PR for the DennisHeimbigner:s3auth.dmh that extends a unit_test and adds a functional test for the SEGFAULT I experienced.
In addition to the patch.

DennisHeimbigner and others added 3 commits January 6, 2026 13:53

Update RELEASE NOTES and fix bad urls

3201eab

Merge branch 'main' into s3auth.dmh

ecd7172

mannreis approved these changes Jan 13, 2026

View reviewed changes

DennisHeimbigner added 2 commits January 13, 2026 11:44

conflicts

f219268

conflict resolution

e52db22

mannreis suggested changes Jan 14, 2026

View reviewed changes

Merge branch 'main' into s3auth.dmh

575c309

mannreis mentioned this pull request Feb 12, 2026

AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables not read for S3/Zarr access #3259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authentication support for session tokens to AWS.#3244

Add authentication support for session tokens to AWS.#3244
DennisHeimbigner wants to merge 6 commits intoUnidata:mainfrom
DennisHeimbigner:s3auth.dmh

DennisHeimbigner commented Jan 6, 2026

Uh oh!

mannreis left a comment

Uh oh!

mannreis Jan 13, 2026

Uh oh!

mannreis Jan 13, 2026

Uh oh!

mannreis Jan 13, 2026

Uh oh!

WardF commented Jan 13, 2026

Uh oh!

DennisHeimbigner commented Jan 13, 2026

Uh oh!

DennisHeimbigner commented Jan 13, 2026

Uh oh!

mannreis left a comment •

edited

Loading

Uh oh!

mannreis Jan 14, 2026

Uh oh!

mannreis Jan 14, 2026

Uh oh!

WardF commented Jan 14, 2026

Uh oh!

mannreis commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	server that requires session tokens. See [Github????](https://github.com/Unidata/netcdf-c/issues/????) for more information.
	server that requires session tokens. See [Github #3136](https://github.com/Unidata/netcdf-c/issues/3136) for more information.

Conversation

DennisHeimbigner commented Jan 6, 2026