Add support for authenticating via client certificate for Openshift #696
Conversation
atomicapp/providers/openshift.py
Outdated
| params = [] | ||
|
|
||
| if self.access_token: | ||
| params = {"access_token": self.access_token} |
There was a problem hiding this comment.
Extra tab given here and why you removing existing value of dict and assigning it only {"access_token": self.access_token}
There was a problem hiding this comment.
that is wrong you are right :-)
|
while the [WIP] text is in the name I assume you guys are still working on this. let me know when you want review. |
Added cert param so that now we can pass client side certs while making request to API server, this can be passed as path to file that contains cert and key or it can be a tuple which has path ti cert and key. This is part of the effort to add support to authenticate to server via cert in addition to access_token.
add support for geting certificates and keys from '-data'
|
#dotests |
kadel
changed the title
| else: | ||
| params = {"access_token": self.access_token} | ||
| if not params: | ||
| params = {} |
There was a problem hiding this comment.
just set params={} in the function declaration. That way {} is the default value given if params isn't provided right? I guess someone could provide params=None though :(
|
Is providing a cert+key in a single file (as mentioned here) a thing? If so are we supporting it with this work? |
|
Also, is there any chance we can bundle this bug in as part of this work: #529 |
| if "certificate-authority" in cluster["cluster"]: | ||
| # if we are in container translate path to path on host | ||
| ca = os.path.join(Utils.getRoot(), | ||
| cluster["cluster"].get("certificate-authority").lstrip('/')) |
There was a problem hiding this comment.
really long line.. can we break it up into two? maybe cluster["cluster"].get("certificate-authority").lstrip('/') on the first and the join on the 2nd.
There was a problem hiding this comment.
"into two" meaning we would need to assignments probably
ca = cluster["cluster"].get("certificate-authority").lstrip('/')
ca = os.path.join(Utils.getRoot(), ca)
There was a problem hiding this comment.
Now when #705 is merged, I'm going to shorten this by using get_real_abspath
There was a problem hiding this comment.
hey @kadel and @dustymabe feel free to look at my implementation def cert_file under https://github.com/projectatomic/atomicapp/pull/712/files (specifically, kubebase.py) for how I implemented the base64 encoding / files and such.
|
Great job guys.. Sorry about all the comments :( - we are really close though |
Temporary files will be garbage collected. Also added a function in Utils to remove files.
|
Shout... I forgot to mention that I already implemented this in See https://github.com/projectatomic/atomicapp/pull/712/files Specifically, |
|
It also includes your issue of base64 encoding (converts files to base64, etc.) as well as cert requests when specifying a certificate. Originally, i was implementing this for the kubernetes api, but because of how similar both the openshift + kubernetes api http implementations are, the cert passing works for both. |
|
@dustymabe yes sir |
|
@dustymabe @cdrage is it time to close this PR? |
fixes #533
TODO: