Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of TimedJSONWebSignatureSerializer #24519

Merged
merged 1 commit into from
Jun 18, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 17, 2022

The TimedJSONWebSignatureSerializer has been deprecated from the
itsdangerous library and they recommended to use dedicated
libraries for it.

pallets/itsdangerous#129

Since we are going to move to FAB 4+ with #22397 where newer version of
itsdangerous is used, we need to switch to another library.

We are already using PyJWT so the choice is obvious.

Additionally to switching, the following improvements were done:

  • the use of JWT claims has been fixed to follow JWT standard.
    We were using "iat" header wrongly. The specification of JWT only
    expects the header to be there and be valid UTC timestamp, but the
    claim does not impact maturity of the signature - the signature
    is valid if iat is in the future.
    Instead "nbf" - "not before" claim should be used to verify if the
    request is not coming from the future. We now require all claims
    to be present in the request.

  • rather than using salt/signing_context we switched to standard
    JWT "audience" claim (same end result)

  • we have now much better diagnostics on the server side of the
    reason why request is forbidden - explicit error messages
    are printed in server logs and details of the exception. This
    is secure, we do not spill the information about the reason
    to the client, it's only available in server logs, so there is
    no risk attacker could use it.

  • the JWTSigner is "use-agnostic". We should be able to use the
    same class for any other signatures (Internal API from AIP-44)
    with just different audience and algorithm

  • Short, 5 seconds default clock skew is allowed, to account for
    systems that have "almost" synchronized time

  • more tests addded with proper time freezing testing both
    expiry and immaturity of the request

This change is not a breaking one because the JWT authentication
details are not "public API" - but in case someone reverse engineered
our claims and implemented their own log file retrieval, we
should add a change in our changelog - therefore newsfragment
is added.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@potiuk potiuk force-pushed the get-rid-of-signature-serializer branch from b60d291 to 3c914fa Compare June 17, 2022 10:09
@potiuk potiuk marked this pull request as ready for review June 17, 2022 10:09
@potiuk
Copy link
Member Author

potiuk commented Jun 17, 2022

This is first change from (likely) a series of changes that will bring us closer to migrating to FAB 4+.

@potiuk potiuk force-pushed the get-rid-of-signature-serializer branch from 3c914fa to 1d3c70d Compare June 17, 2022 10:18
@potiuk potiuk mentioned this pull request Jun 17, 2022
1 task
airflow/utils/jwt_signer.py Outdated Show resolved Hide resolved
airflow/utils/serve_logs.py Outdated Show resolved Hide resolved
airflow/utils/serve_logs.py Outdated Show resolved Hide resolved
@potiuk
Copy link
Member Author

potiuk commented Jun 18, 2022

All fixed @mik-laj

@potiuk potiuk force-pushed the get-rid-of-signature-serializer branch 3 times, most recently from 7af500f to 55bf6e2 Compare June 18, 2022 09:55
@potiuk
Copy link
Member Author

potiuk commented Jun 18, 2022

I also added algorithm constructor argument, as we will likely use different algorithm (asymmetric RS) in the future for Internal API

airflow/utils/serve_logs.py Show resolved Hide resolved
airflow/utils/serve_logs.py Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the get-rid-of-signature-serializer branch from 55bf6e2 to 20158f0 Compare June 18, 2022 10:05
The TimedJSONWebSignatureSerializer has been deprecated from the
itsdangerous library and they recommended to use dedicated
libraries for it.

pallets/itsdangerous#129

Since we are going to move to FAB 4+ with apache#22397 where newer version of
itsdangerous is used, we need to switch to another library.

We are already using PyJWT so the choice is obvious.

Additionally to switching, the following improvements were done:

* the use of JWT claims has been fixed to follow JWT standard.
  We were using "iat" header wrongly. The specification of JWT only
  expects the header to be there and be valid UTC timestamp, but the
  claim does not impact maturity of the signature - the signature
  is valid if iat is in the future.
  Instead "nbf" - "not before" claim should be used to verify if the
  request is not coming from the future. We now require all claims
  to be present in the request.

* rather than using salt/signing_context we switched to standard
  JWT "audience" claim (same end result)

* we have now much better diagnostics on the server side of the
  reason why request is forbidden - explicit error messages
  are printed in server logs and details of the exception. This
  is secure, we do not spill the information about the reason
  to the client, it's only available in server logs, so there is
  no risk attacker could use it.

* the JWTSigner is "use-agnostic". We should be able to use the
  same class for any other signatures (Internal API from AIP-44)
  with just different audience

* Short, 5 seconds default clock skew is allowed, to account for
  systems that have "almost" synchronized time

* more tests addded with proper time freezing testing both
  expiry and immaturity of the request

This change is not a breaking one because the JWT authentication
details are not "public API" - but in case someone reverse engineered
our claims and implemented their own log file retrieval, we
should add a change in our changelog - therefore newsfragment
is added.
@potiuk potiuk force-pushed the get-rid-of-signature-serializer branch from 20158f0 to 6d2653c Compare June 18, 2022 10:20
@potiuk
Copy link
Member Author

potiuk commented Jun 18, 2022

Right. Fixed and reviewed all.

@potiuk
Copy link
Member Author

potiuk commented Jun 18, 2022

BTW. Interesting dicussion here https://bugs.python.org/issue46200

@potiuk
Copy link
Member Author

potiuk commented Jun 18, 2022

All should be cool @mik-laj :)

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 18, 2022
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk merged commit 1f8e4c9 into apache:main Jun 18, 2022
@potiuk potiuk deleted the get-rid-of-signature-serializer branch June 18, 2022 21:08
@potiuk potiuk mentioned this pull request Jun 22, 2022
potiuk added a commit to potiuk/airflow that referenced this pull request Jun 29, 2022
The TimedJSONWebSignatureSerializer has been deprecated from the
itsdangerous library and they recommended to use dedicated
libraries for it.

pallets/itsdangerous#129

Since we are going to move to FAB 4+ with apache#22397 where newer version of
itsdangerous is used, we need to switch to another library.

We are already using PyJWT so the choice is obvious.

Additionally to switching, the following improvements were done:

* the use of JWT claims has been fixed to follow JWT standard.
  We were using "iat" header wrongly. The specification of JWT only
  expects the header to be there and be valid UTC timestamp, but the
  claim does not impact maturity of the signature - the signature
  is valid if iat is in the future.
  Instead "nbf" - "not before" claim should be used to verify if the
  request is not coming from the future. We now require all claims
  to be present in the request.

* rather than using salt/signing_context we switched to standard
  JWT "audience" claim (same end result)

* we have now much better diagnostics on the server side of the
  reason why request is forbidden - explicit error messages
  are printed in server logs and details of the exception. This
  is secure, we do not spill the information about the reason
  to the client, it's only available in server logs, so there is
  no risk attacker could use it.

* the JWTSigner is "use-agnostic". We should be able to use the
  same class for any other signatures (Internal API from AIP-44)
  with just different audience

* Short, 5 seconds default clock skew is allowed, to account for
  systems that have "almost" synchronized time

* more tests addded with proper time freezing testing both
  expiry and immaturity of the request

This change is not a breaking one because the JWT authentication
details are not "public API" - but in case someone reverse engineered
our claims and implemented their own log file retrieval, we
should add a change in our changelog - therefore newsfragment
is added.

(cherry picked from commit 1f8e4c9)
@ephraimbuddy ephraimbuddy added this to the Airflow 2.3.3 milestone Jun 30, 2022
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging full tests needed We need to run full set of tests for this PR to merge type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants