support "user_claim_json_pointer" in create_role() for JWT/OIDC auth method

[ferenc-hechler]

This is a follow-up to PR #998 which was broken due to my attempts to switch from develop to main branch.

When creating a role for the JWT auth method, the optional parameter "user_claim_json_pointer" is missing.
See documentation here:

https://developer.hashicorp.com/vault/api-docs/auth/jwt#user_claim_json_pointer

The parameter is a bool value which defaults to false.
This pull request adds the missing parameter. I tested it for JWT auth and it works.

The OIDC auth mehod inherits create_role() from JWT.
So, I also added this parameter to ODIC.create_role().
The OIDC use case was not tested.

Documentation was updated.

[briantist]

Apologies for issues on the branch changes, there should not have been any need to rename or change your original branch on the other PR.

Though it is recommended to always create a new branch in your fork for your PRs, instead of using the default branch.

Thanks for sticking with us through it all!

[ferenc-hechler]

One info about "make test".
I am working with Windows 10 and all tests failed because the vault server was not started correctly from the server-manager:

hvac/tests/utils/server_manager.py

Lines 57 to 59 in 31aca14

	process = subprocess.Popen(
	command, stdout=subprocess.PIPE, stderr=subprocess.PIPE
	)

Without understanding the reasons, when I removed the redirect of stdout and stderr, which means, the server output is displayed together with the test output, then it worked. :-)

            process = subprocess.Popen(
                #command, stdout=subprocess.PIPE, stderr=subprocess.PIPE
                command
            )

the jwt and oidc tests were executed successfully.
I did not add or change the existing tests.

[ferenc-hechler]

Apologies for issues on the branch changes, ...

No problem. The few changes were quickly reapplied to the main branch.

[briantist]

Thank you very much for reporting the problem with running the tests locally, I've opened a separate issue for that, please feel free to subscribe to it or add any additional info you might have:

• Integration tests can't be run on Windows #1007

[briantist]

Just some minor docstring changes but otherwise it looks good to me. Since the new parameter was added in Vault 1.11.x I was worried that maybe it would fail against older versions but the integration tests seemed to pass on those earlier versions.

I do wish we had more testing the of the parameter itself, but I don't have a lot of experience with OIDC/JWT auth in Vault. If you have an idea how we could add some actual tests for that it'd be great.

[ferenc-hechler]

... would fail against older versions ...

uhhh, good point. Thinking about this I also would expect the old version to fail. It depends on how unknown parameters are treated.

I do wish we had more testing the of the parameter itself, but I don't have a lot of experience with OIDC/JWT auth in Vault. If you have an idea how we could add some actual tests for that it'd be great.

I can add some tests. But the current JWT used in the test is flat, no hierarchical values, which would need JSON Pointer. So, I also have to change this.
But to be clear: What would be tested is the server functionality, because there is no logic behind in the Python implementation.

[briantist]

... would fail against older versions ...

uhhh, good point. Thinking about this I also would expect the old version to fail. It depends on how unknown parameters are treated.

I do wish we had more testing the of the parameter itself, but I don't have a lot of experience with OIDC/JWT auth in Vault. If you have an idea how we could add some actual tests for that it'd be great.

I can add some tests. But the current JWT used in the test is flat, no hierarchical values, which would need JSON Pointer. So, I also have to change this. But to be clear: What would be tested is the server functionality, because there is no logic behind in the Python implementation.

I spent some more time looking at the test implementation yesterday, and to be honest I'm having trouble following it. I am wondering if the reason it didn't fail on the earlier versions of Vault is because some part of the process is being mocked out, but I wasn't able to tell definitively.

As you said, adding an integration test is testing the server and not really our code; I am most interested in ensuring that 1) the code as written (without guarding for version number in our logic) will work with the earlier versions of Vault we still support, and 2) that our code consistently passes the parameters through, to prevent regressions.

For 2), I would prefer that be in a unit test, but no unit tests exist for this auth method yet, and that isn't being tested for any other parameters, so I cannot expect that to be a blocker for this PR. If you are interested in adding units, that would be very appreciated but not required. In that case I would say to prefer pytest style rather UnitTest style which is currently in use, unless using UnitTest style would be easier due to it already being in the project as an example.

If you are able to manually test this change against our oldest CI version of Vault (even a curl / Invoke-RestMethod that sets uses the new parameter), that at least gives us a clue as to whether we need to be worried about version. If we can confirm we don't, then I'm inclined to accept the change as is if untangling the integration tests proves to be too burdensome.

[ferenc-hechler]

I run the jwt integration test against a manually started vault and I can see, that the create_role request is received.
The role is created correctly.

I run the testsuite against a vault version 1.4.7 without any problems.
The role is created and the known parameters are set:

vault read /auth/jwt-test/role/hvac

Key                        Value
---                        -----
allowed_redirect_uris      [https://localhost:8200/jwt-test/callback]
bound_audiences            [1234]
bound_claims               <nil>
bound_claims_type          string
bound_subject              n/a
claim_mappings             <nil>
clock_skew_leeway          0
expiration_leeway          0
groups_claim               n/a
not_before_leeway          0
oidc_scopes                <nil>
role_type                  jwt
token_bound_cidrs          []
token_explicit_max_ttl     0s
token_max_ttl              0s
token_no_default_policy    false
token_num_uses             0
token_period               0s
token_policies             []
token_ttl                  0s
token_type                 default
user_claim                 https://vault/user
verbose_oidc_logging       false

The two parameters user_claim_json_pointer and max_age are missing, because they were introduced later, but all other parameters are set correctly.

Also tested with curl to add an "unknown_parameter":

set DATA="{\"unknown_parameter\": \"xxxx\", \"name\": \"hvac\", \"role_type\": \"jwt\", \"bound_audiences\": [\"1234\"], \"user_claim\": \"https://vault/user\", \"allowed_redirect_uris\": [\"https://localhost:8200/jwt-test/callback\"], \"bound_claims_type\": \"string\", \"verbose_oidc_logging\": false, \"user_claim_json_pointer\": false}"

set TOKEN=s.fKl5ahzHbbzqscuAkeucxHos

curl -k -XPOST -d "%DATA%" -H "X-Vault-Token: %TOKEN%" -H "Content-Type: application/json" -H "X-Vault-Request: true" https://localhost:8200/v1/auth/jwt-test/role/hvac

This works without problems. So, unknown keys are just ignored.

[ferenc-hechler]

About adding pytests: I am not experienced with this. Do you have a reference, which I can use as a template.
Otherwise I will use the existing unittests as template.

[ferenc-hechler]

I applied your proposed changes and created an initial unit test for jwt validating, that the parameter "user_claim_json_pointer" is contained in the http request according to the set value.

Also I changed the default value for user_claim_json_pointer to None, so it behaves identical to the parameter "token_no_default_policy", which is also of type bool and the documented default value is "false". But the default value is set on server side and not in the python sdk. Also this ensures another layer of compatibility. If the parameter is "None" it will not be submitted in the request. So, even if unknown parameters are checked in older versions, as long, as it is not set, it is compatible.

[codecov]

Codecov Report

Merging #1006 (cbc1e70) into main (d5eabd5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1006   +/-   ##
=======================================
  Coverage   84.70%   84.70%           
=======================================
  Files          65       65           
  Lines        3066     3066           
=======================================
  Hits         2597     2597           
  Misses        469      469           

Impacted Files	Coverage Δ
hvac/api/auth_methods/jwt.py	92.30% <ø> (ø)
hvac/api/auth_methods/oidc.py	100.00% <ø> (ø)

[briantist]

I think we're good to go here. Thanks again @ferenc-hechler for the contribution (and adding tests) and your patience! Thanks to @adammike for helping me out with review .