Skip to content
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

add natsOptions_SetUserCredentialsFromMemory #621

Merged
merged 1 commit into from
Jan 5, 2023
Merged

add natsOptions_SetUserCredentialsFromMemory #621

merged 1 commit into from
Jan 5, 2023

Conversation

MichaelVoelkel
Copy link
Contributor

I would say, this is very useful (I need it myself) because I am actually getting the JWT as env var in some docker container. I could write it to a file and then read that but it feels like unnecessary file I/O so I would rather directly read from memory.

I tested it personally and it works.

I can add UTs if you generally would accept this change (otherwise I'd rather save the time :)).

@kozlovic kozlovic changed the base branch from main to dev January 2, 2023 19:48
@kozlovic kozlovic changed the base branch from dev to main January 2, 2023 19:58
@kozlovic kozlovic changed the base branch from main to dev January 2, 2023 19:58
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Since this would be a new API, I would like you to rebase your PR from the dev branch and re-post. When you submit a PR you can select the base branch to submit against.

But also, I would prefer that we extend the userCreds struct and add a new field that would be a copy of the memory passed to the new API. But otherwise it would be very similar to natsOptions_SetUserCredentialsFromFiles(). I would simply rename natsConn_userFromFile to natsConn_userCreds or something like that and check for the new field in _sign() and the renamed natsConn_userFromFile. In other words, I think we can get away with the single new option API. Could you try?

src/nats.h Outdated
/** \brief Sets JWT handler and handler to sign nonce that uses seed.
*
* This function acts similarly to natsOptions_SetUserCredentialsFromFiles but reads from memory instead
* from a file. Also it assumes that `memory` contains both the JWT and NKey seed. Note that the format of memory is:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the sentence is not finished.

@MichaelVoelkel
Copy link
Contributor Author

MichaelVoelkel commented Jan 3, 2023

Since this would be a new API, I would like you to rebase your PR from the dev branch and re-post. When you submit a PR you can select the base branch to submit against.

But also, I would prefer that we extend the userCreds struct and add a new field that would be a copy of the memory passed to the new API. But otherwise it would be very similar to natsOptions_SetUserCredentialsFromFiles(). I would simply rename natsConn_userFromFile to natsConn_userCreds or something like that and check for the new field in _sign() and the renamed natsConn_userFromFile. In other words, I think we can get away with the single new option API. Could you try?

Understood and why not! Will check!

@MichaelVoelkel
Copy link
Contributor Author

MichaelVoelkel commented Jan 4, 2023

Okay, so this version also has some test. I just assumed it's fine to put it next to the tests for UserCredsFromFiles...

Whenever I push here, some tests fail. Are some flaky or is this expected? Right now linters seem fine but one test fails (which seems unrelated)

Also, would you like to see an addition to examples.h?

What could also be improved would be better error messages in case of failed connection or wrong parameter detection upfront. I'm not so sure if the client has this everywhere, but I could add a bit about it if you want?

And glad to receive any other feedback clearly.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Thanks Michael. This is a very good PR. There is just a small bug checking for memory allocation failure, and you need to include the new test in the test suite list by following the instructions that I have directed you too (I did it locally and your new test passes and does not have memory leak, but as of now it is not run in the PR itself).
Also, I would like you to squash to a single commit when I will approve and before I merge. Thanks again!

src/opts.c Outdated
uc->userOrChainedFile = NATS_STRDUP(uocf);
if (uc->userOrChainedFile == NULL)
uc->jwtAndSeedContent = NATS_STRDUP(jwtAndSeedContent);
if (uc == NULL)
Copy link
Member

Choose a reason for hiding this comment

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

It should be if (uc->jwtAndSeedContent == NULL) here.

test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Outdated
@@ -34137,6 +34260,7 @@ static testInfo allTests[] =
{"GetLocalIPAndPort", test_GetLocalIPAndPort},
{"UserCredsCallbacks", test_UserCredsCallbacks},
{"UserCredsFromFiles", test_UserCredsFromFiles},
{"UserCredsFromMemory", test_UserCredsFromMemory},
Copy link
Member

Choose a reason for hiding this comment

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

This test that you added was actually not included in the suite. You need to run test/testsuite which will produce an updated test list and move it to the test/ directory and commit this change. See instructions here

@MichaelVoelkel
Copy link
Contributor Author

MichaelVoelkel commented Jan 5, 2023

Forgot to push yesterday evening. Anyways, some things to mention with current behaviour:

  • if natsOptions_SetUserCredentialsFromMemory is called with content NULL, then the content will be cleared
  • if not, then the content will first be checked (i.e. does it have two items)... as this is content that should be nulled, there exists natsCrypto_Clear which I used but it means another header include is necessary
  • for natsOptions_SetUserCredentialsFromFiles, the check of files is not done until the connect is actually made... that is why originally I did it the same way. If the function needs to check the two lines, it's a bit more effort and also different behaviour from natsOptions_SetUserCredentialsFromFiles - fine for me, just wanted to point it out

The tests mirror the behaviour, so it can be observed. (tests still failing... one second)

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Sorry about that. You are right, we want to be consistent, so let's have the test attempt the connect instead.

src/opts.c Outdated Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution!

@kozlovic kozlovic merged commit 6d22e57 into nats-io:dev Jan 5, 2023
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.

2 participants