fix vault client certificates loaded from envirnoment variables

[BrandonHoffman]

It seems like the hvac library does not handle the VAULT_CLIENT_CERT and VAULT_CLIENT_KEY variables properly.

• public/private key-pairs in requests need to be passed as a tuple
• environment variables are overriding the values directly passed in

Fixes #938
Fixes #787

[BrandonHoffman]

#938

and

#787

are both related

[briantist]

@BrandonHoffman welcome and thanks for submitting this! I've been away traveling but I intend to take a look at the backlog some more after I get back and get caught up on other things, so it might take a little while but I will review this!

In the meantime run petry run black . and poetry run flake8 . to check for some formatting lint errors.

[BrandonHoffman]

@briantist - no worries, just know we were hitting issues with this in my org and figured it would effect others. The fmt/lint issue is fixed.

[BrandonHoffman]

also fixed an integration test issue related to integration tests running in python 3.7 which does not support the nested with syntax I initially used.

[adammike]

I can't imagine how that original code worked, with the list split into a string like that. @briantist I've reviewed this. The code looks good, and I've validated that what's being passed to the adapter is what requests is expecting.

Rather than change all tests to use the environment, we should add new tests for this use case specifically - mTLS authentication with the certs passed to the client as parameters, and mtls with the certs passed in the environment.

Another issue I have, and this is not aimed at this committer, we are implicitly passing authentication parameters from the adapter to the requests session. While functional and technically not a bug, Its best practice to be implicit with authentication and authorization parameters.

[adammike]

The issue with the implicit auth parameters does not need to be fixed in this PR, just pointing out the issue. It should probably be tracked as a security enhancement bug.

[BrandonHoffman]

Hey @adammike,

toggling everything to use the environment variables was actually a mistake...... accidentally checked in the global toggle to true that I used locally just to validate everything was working with this flow.

I actually only intended for this one subclass to be the test case for using the environment variables since it seemed like it would be enough to validate this flow.

https://github.com/hvac/hvac/pull/943/files#diff-f12f973ead75ad35d6886ad63a746f8a3c2f540b4ea538845ac6346353a65a74R158

[adammike]

Looks good to me!

[codecov]

Codecov Report

Merging #943 (c765e42) into develop (b11a2ec) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #943      +/-   ##
===========================================
+ Coverage    83.60%   83.67%   +0.06%     
===========================================
  Files           65       65              
  Lines         3007     3007              
===========================================
+ Hits          2514     2516       +2     
+ Misses         493      491       -2     

Impacted Files	Coverage Δ
hvac/v1/__init__.py	88.53% <100.00%> (+1.27%)	⬆️

[briantist]

This all looks good! Thanks for this fix @BrandonHoffman .

The only thing I want to consider before merging is whether we're able to release this in a minor version or whether we should wait until a major out of an abundance of caution.

It's technically a breaking change. It should only hit someone who:

• is passing values for these both directly and in environment variables
• is passing the desired values in environment variables
• is passing undesired values directly

While the remedy is clear for such a use case, the change is still unexpected for that user.

In any case we'll get this out as soon as we can!

[briantist]

@adammike is there anything else you'd like to review here?
If not, could you approve or dismiss your previous review?

Any thoughts on the breaking change consideration I posted above?

[briantist]

Fixes #938
Fixes #787

@BrandonHoffman @adammike I don't see any reason why merging this shouldn't close the issues above, do you agree?

[BrandonHoffman]

So, @briantist you’re a lot more familiar with the code base then I am. But here is my 2 cents, from what I have seen any case where the environment variables are set results in a traceback so I don’t believe that anyone using the library as it is now could use the environment variable without having A trace back occur. That leads me to believe it should be safe, but maybe there is some edge case I’m not considering.

[BrandonHoffman]

I think both those issues should be addressed with this.

[briantist]

from what I have seen any case where the environment variables are set results in a traceback so I don’t believe that anyone using the library as it is now could use the environment variable without having A trace back occur. That leads me to believe it should be safe, but maybe there is some edge case I’m not considering.

Ah that's right, I forgot that the env code as is wouldn't have worked anyway, so I think you're right, and that we can safely call this a bugfix, thanks again!

[BrandonHoffman]

Yeah no problem thank you for being so welcoming.

[dhuckins]

packaging is a dep for various packages but should still add it as a dev dep

[briantist]

good catch! Added in c765e42

[adammike]

LG2M