Add support for IAM token #10939
Add support for IAM token #10939
Conversation
|
Jenkins results:
|
|
@amaltaro , @todor-ivanov could you please review this PR. The changes are trivial and if merged we can start testing usage of WMCore services with token based authentication. |
There was a problem hiding this comment.
Valentin, in addition to the comments made along the code, I have a more general question/comment on how these tokens will be used, like:
- once we load a token in the
RequestHandlerobject, I think we should keep using it instead of making the os.environ call for every HTTP call that goes through pycurl_manager. - with that said, we would need to have a mechanism to also update our token because it will eventually expire, right?
- adding support to reading this token from a file might be a good idea
Last but not least, the pycurl_manager module also contains some functions. We need to make sure getdata function is covered with this implementation.
In terms of other HTTP modules, we use this one for uploading a root file - from within the worker node - to the DQM Gui:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/Services/HTTPS/HTTPSAuthHandler.py
you might want to have a look at it as well.
| @@ -61,6 +61,14 @@ | |||
| from Utils.PortForward import portForward, PortForward | |||
|
|
|||
|
|
|||
| def get_token(env=None): | |||
There was a problem hiding this comment.
Valentin, I believe at some point we will want to have different operations for tokens (like, renewing, checking validity, etc) and maybe even different technologies. Right?
In that case, I would suggest to create a new module under the Utils package and have all the token-based implementation in there.
| @@ -242,6 +257,7 @@ def set_opts(self, curl, url, params, headers, | |||
| if verbose: | |||
| curl.setopt(pycurl.VERBOSE, True) | |||
| curl.setopt(pycurl.DEBUGFUNCTION, self.debug) | |||
| self.request_headers = headers | |||
There was a problem hiding this comment.
Given that this attribute is not used anywhere but here in the __init__ method, it looks like this line could be removed.
There was a problem hiding this comment.
the request headers can be required in different clients, e.g. DBSClient, and should be set accodringly. This is why it was set here. The are initially declared in ctor and set appropriate for every HTTP request.
There was a problem hiding this comment.
If it's using this class, then it should already have the headers set (L239), no? Which DBSClient library are you referring to here?
Maybe to verify whether it's really needed or not, you could comment this out and push it in and see what jenkins output we get.
|
@amaltaro thanks for comments. We're working on setting up never expired token, similar to how we handle proxy for x509. Once new service will be in place, the token will be updated on regular basis and placed in a file. The token validity is defined by IAM provider and as far as I saw it is valid for 1hour. Therefore, I'll adjust code to do the following:
I'll put new code in Utils as you suggested and then use it here and pycurl manager. |
|
@amaltaro , I provided a first draft of TokenManager which so far can only read and validate token. Later, we may discuss and add new functionality how to obtain new token in TokenManager. Please bare this in your review. I explored several python libraries and decided to stick to pyjwt recommended on jwt.io. This library provides ability to inspect token and does not have much dependencies. This implies that it should be added to requirements of WMCore. |
|
Jenkins results:
|
|
I fixed unit tests, but we should decide how to perform unit test which require valid token. I put necessary comments into the codebase to comeback to this once we'll implement procedure to obtain token programmatically. |
|
Jenkins results:
|
|
@amaltaro , @todor-ivanov I am not sure I understand the following pylint error |
|
@vkuznet I haven't looked into this PR, but please replace the print statement by the logging library. This is an effort that we made a few years ago and kept the print statement only where we didn't manage to get a logger object working well. |
|
Jenkins results:
|
|
Jenkins results:
|
|
@vkuznet Valentin, could you please work on the cmsdist specs to get this new python library in the stack? Once you have that working, please link it to the first description of this PR. I haven't looked into these details, but I believe the massive failures are coming from the import error for the new library. |
|
@amaltaro I made necessary changes to replace print with logger. I checked that all pylint issues are gone. But the jenkins tests are failing due to the following: It seems to me that in Jenkins there is an issue with environment since Therefore, it seems to me that there is an issue with loading pycurl_manager which is part of this PR. Since there is not any printout in import error section we don't know what is the actual problem. Locally, I can easily load pycurl_manager and its pylint scores 10/10. If you want I can change |
vkuznet
force-pushed
the
iam-token
branch
2 times, most recently
from
ff0f581 to
deddb15
Compare
|
Jenkins results:
|
|
@amaltaro , please be specific which spec file you're talking about? I adjusted code base to handle missing |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
There was a problem hiding this comment.
Valentin, I had another look at this and it's looking good now, almost ready to go.
I resolved a few comments that you have already worked on but were not automatically closed by GH, and left 2 or 3 comments that are worth it.
Once you have those changes in place, feel free to rebase and squash these commits. If it's not too late, please keep the test changes in a separate commit.
src/python/Utils/TokenManager.py
Outdated
| It caches token along with expiration timestamp. | ||
| By default the env variable to use is IAM_TOKEN. | ||
| :param name: string representing either file or env where we should read token from | ||
| :param url: IAM provider URL, by default it is https://cms-auth.web.cern.ch/jwk |
There was a problem hiding this comment.
Regarding this comment, please actually consider the one made in the __init__ method signature.
|
@vkuznet Valentin, can you also please give a brief summary of what is provided in this PR and what kind of token management it allows now? It'd be useful to link the WMCore token wiki as well: https://github.com/dmwm/WMCore/wiki/Tokens-in-WMCore FYI @belforte @mapellidario this change is likely going in in the next release and it depends on a new python 3rd-party library called: Last but not least, these changes - especially to pycurl_manager - should be backwards compatible, so even if we are not using tokens and don't have |
|
Jenkins results:
|
|
@amaltaro , thanks for suggestions I adjusted the codebase accordingly. Regarding PR description. I updated it too. But this PR does not provide any token management, we only provide methods (either via function or class) to read token, but we do not manage tokens. Pointer to wiki has been added. |
|
Jenkins results:
|
|
Thank you, Valentin. Yes, by management I meant that we have some utilities to load in a token and check expiration time. But I agree this is only a small set of managing tokens. If you are ready with these changes, please:
|
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
|
thanks for keeping us in the loop. Appreciated. |
Fixes #10940
This PR provides support to include IAM token to HTTP requests. The token can be obtained via separate process, e.g. via curl,
create-iam-token.shscript or oidc-agent, and then can be used by any HTTP client. We added appropriateAuthorizationHTTP header topycurl_manager. The token itself can be provided either via file name or environment variable.For more information please consult Tokens in WMCore wiki page.
Status
ready
Description
We provide a patch to pycurl manager to setup appropriate HTTP header if token is provided via configuration or environment. And,
TokenManagermodule which provides different ways to obtain IAM token, either viareadTokenfunction orTokenManagerclass.Is it backward compatible (if not, which system it affects?)
YES
Related PRs
Here is PR for wmcore/wmagent specs: cms-sw/cmsdist#7647
External dependencies / deployment changes
None