Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Fix base64 padding for kube config #79

Merged
merged 5 commits into from
Jun 19, 2019
Merged

Fix base64 padding for kube config #79

merged 5 commits into from
Jun 19, 2019

Conversation

bpicolo
Copy link
Contributor

@bpicolo bpicolo commented Jul 30, 2018

Fixes #65 (pulled in / updated another open PR to get this fix moving + tweaked tests)

JWT spec requires tokens be de-padded base64 (so that they're URLsafe), but they need to be re-padded to properly decode.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 30, 2018
@codecov-io
Copy link

codecov-io commented Jul 30, 2018

Codecov Report

Merging #79 into master will increase coverage by 0.02%.
The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   92.11%   92.13%   +0.02%     
==========================================
  Files          13       13              
  Lines        1217     1234      +17     
==========================================
+ Hits         1121     1137      +16     
- Misses         96       97       +1
Impacted Files Coverage Δ
config/kube_config_test.py 94.55% <100%> (+0.13%) ⬆️
config/kube_config.py 83.9% <91.66%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e077f88...e1a1d3d. Read the comment docs.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 30, 2018
@bpicolo bpicolo closed this Jul 30, 2018
@bpicolo bpicolo reopened this Jul 30, 2018
@zybjcdl
Copy link

zybjcdl commented Aug 22, 2018

@bpicolo When could we get this fix? We need to run with python 3.6.5

@bpicolo
Copy link
Contributor Author

bpicolo commented Aug 27, 2018

/assign @roycaihw . (as recommended by mrsbot)

if PY3:
jwt_attributes = json.loads(
base64.b64decode(parts[1]).decode('utf-8')
base64.b64decode(parts[1] + padding).decode('utf-8')
Copy link
Member

@roycaihw roycaihw Aug 27, 2018

Choose a reason for hiding this comment

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

JWT requires base64.urlsafe_b64decode instead of base64.b64decode (and for encoding as well), as + and / are not url safe

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be still worthwhile to do safe decode here?

@@ -87,11 +91,13 @@ def _raise_exception(st):

TEST_OIDC_TOKEN = "test-oidc-token"
TEST_OIDC_INFO = "{\"name\": \"test\"}"
TEST_OIDC_BASE = _base64(TEST_OIDC_TOKEN) + "." + _base64(TEST_OIDC_INFO)
TEST_OIDC_BASE = _unpadded_base64(
TEST_OIDC_TOKEN) + "." + _unpadded_base64(TEST_OIDC_INFO)
TEST_OIDC_LOGIN = TEST_OIDC_BASE + "." + TEST_CLIENT_CERT_BASE64
Copy link
Member

Choose a reason for hiding this comment

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

The signature (TEST_CLIENT_CERT_BASE64) should also be url safe (base64url encoded and padding omitted).

@@ -257,13 +257,15 @@ def _load_oid_token(self, provider):
if len(parts) != 3: # Not a valid JWT
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a check to make sure the JWT doesn't contain url reserved characters (=, + and /)?

Copy link
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR :) I left some comments.

@joppa27 joppa27 mentioned this pull request Sep 24, 2018
@roycaihw
Copy link
Member

After more reading, I'm thinking if the previous padding implementation (padding with "==") was intended.

According to base64 encoding and JWS doc, base64 (also apply to base64url) padding can only have three cases:

  • no padding
  • two "=" padding characters
  • one "=" padding character

Having three "=" padding characters is illegal.

According to https://gist.github.com/perrygeo/ee7c65bb1541ff6ac770 and python binascii_a2b_base64 implementation, extra paddings after previous quad are ignored. Therefore for a valid base64-encoded string with padding stripped, adding "==" would guarantee it can be safely decoded, and for an illegal string (e.g. "A===" being "A" when stripped) adding "==" would raise error.

In Python 3.7 the implementation distinguishes illegal string ("A==") from incorrect padding ("AQ="): https://github.com/python/cpython/blob/3.7/Modules/binascii.c#L513-L521. Prior 3.7 the error messages are the same "Incorrect padding".

#65 had this issue because we didn't implement padding for python 3. The padding implementation for python 2 was correct (probably intended?) but it didn't distinguish illegal string from incorrect padding. I suggest we could add illegal base64url string check like https://tools.ietf.org/html/rfc7515#appendix-C

cc @ltamaster who implemented oidc auth
cc @hanikesn @joppa27 who also contributed on this fix

@bpicolo
Copy link
Contributor Author

bpicolo commented Sep 25, 2018

@roycaihw fair - I didn't notice the addendum in the JWS RFC. What's the optimal path forward?

@roycaihw
Copy link
Member

@bpicolo I think your fix is fine for both Python 2 and 3. I'd suggest that we can add a check and raise error if len(padding) == 3, so that the versions prior 3.7 can better distinguish malformed token and incorrect padding.

Please also fix the decoding and testcases around oidc id-token (JWT) to use base64url-encoding/decoding.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 18, 2019
@bpicolo
Copy link
Contributor Author

bpicolo commented Feb 18, 2019

@roycaihw Had the first chance/attention span in, clearly quite a while, to address the things we talked about long ago, because I get emails about this on occasion from people referencing it, heh.

The actionables I tweaked:

  • Fail if padding length is 3
  • Fail if reserved characters are present
  • Urlsafe test b6eencode

Hopefully this is good to go, though a potentially useful follow up might be noisier auth errors

@micw523
Copy link
Contributor

micw523 commented Feb 19, 2019

Hi @bpicolo -
It looks like that your tests are somewhat incomplete and decreased the code coverage for the bot. It is possible for you to add some tests that you mentioned, e.g. if padding length is 3, etc?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 19, 2019
@bpicolo
Copy link
Contributor Author

bpicolo commented Feb 19, 2019

@micw523 Done.

Where is code coverage report now? Last one I see in this thread ran in July on an entirely different branch

@micw523
Copy link
Contributor

micw523 commented Feb 19, 2019

I think the bot refreshes its comment every once a while - it should refresh itself.

Otherwise, the CI says your coverage is 92%.

@bpicolo
Copy link
Contributor Author

bpicolo commented Feb 19, 2019

Ahhh I see that now, gotcha.

Side note - these tests are super tough to get running locally (And the latest kubernetes/ seems to have a dependency glitch with adal so I had to edit the ./run_tox script to target a specific branch locally), and because they run in a bash subprocess they're even harder to debug. It's probably worth adding some documentation to help people get started here.

That said, looks like coveragebot is happy now?

@micw523
Copy link
Contributor

micw523 commented Feb 19, 2019

Are you running into import problems with adal? I think part of the reason might be that you opened this PR a long time ago and the python repo has ditched adal as an absolute requirement. It was a recent change. Fast forwarding to the master branch might help?

I agree on the difficulty on running the tests. The documentation for this library is quite sparse in its current state.

The bot has indeed updated itself.


if any(char in token for char in reserved_characters):
# Invalid jwt, as it contains url-unsafe chars
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like that in the original code a single return is used. Is it possible if we make the return statements consistent?

if PY3:
jwt_attributes = json.loads(
base64.b64decode(parts[1]).decode('utf-8')
base64.b64decode(parts[1] + padding).decode('utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be still worthwhile to do safe decode here?

@bpicolo
Copy link
Contributor Author

bpicolo commented Mar 30, 2019

@micw523 cleaned up the return. re: safe decode (can't reply to that comment), we're breaking above if there are url-unsafe characters, so it should be safe as is.

In theory, this is ready, but I no longer have a setup locally to be able to try it live. Apologies for the delay here - pretty far out of my mind at this point, heh.

Not sure what's up with those new test errors ... doesn't seem code related?

@stevenmanton
Copy link

Just pinging this thread to see if there are updates. This bug still blocks our development without manually editing the offending line. Is this PR just awaiting lgtm?

@roycaihw
Copy link
Member

Sorry for the delay in review. I will do another pass this week

@carlescere
Copy link

carlescere commented Jun 13, 2019

Any news on this?

@roycaihw
Copy link
Member

my apologies.. I was busy with k/k 1.15 freeze. It looks like my comments were addressed a long time ago :)

re: safe decode (can't reply to that comment), we're breaking above if there are url-unsafe characters, so it should be safe as is.

Fair enough. I re-ran the travis CI and the test_create_apiservice_from_yaml_with_conflict failure looks unrelated. I tend to merge this. Thanks for the fix!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bpicolo, roycaihw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2019
@roycaihw roycaihw merged commit 487c918 into kubernetes-client:master Jun 19, 2019
@rcurrie
Copy link

rcurrie commented Jul 3, 2019

Just installed released 10.0.0 fresh from pip and tested against the Pacific Research Platform k8s cluster using python3 and it works now! Thank You!

k8s-ci-robot added a commit that referenced this pull request Jul 30, 2019
…stream-release-9.0-base64-patch

Automated cherry pick of #79
k8s-ci-robot added a commit that referenced this pull request Jul 30, 2019
…stream-release-8.0-base64-patch

Automated cherry pick of #79
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC auth uses incorrect base64 decoding
9 participants