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

Add support for TLSv1.3 in nginx configurations #18659

Merged
merged 2 commits into from
May 26, 2023

Conversation

malmor
Copy link
Contributor

@malmor malmor commented May 9, 2023

Thank you for contributing to Harbor!

Comprehensive Summary of your change

This PR enables TLSv1.3 by default in the nginx configuration. This way the configuration follows the linked recommendations and the nginx default configuration.

Issue being fixed

Fixes #18358

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@malmor malmor marked this pull request as ready for review May 9, 2023 15:49
@malmor malmor requested a review from a team as a code owner May 9, 2023 15:49
@MinerYang MinerYang self-assigned this May 10, 2023
@MinerYang
Copy link
Contributor

Hi @malmor ,
Could you please help to rebase and resolve the conflict since the notary has been removed from main
Thanks!

@malmor malmor force-pushed the 18358-add-tls-1-3-support branch from 44f9b2b to 4feddcb Compare May 24, 2023 18:16
@malmor
Copy link
Contributor Author

malmor commented May 24, 2023

Hey @MinerYang,
sure - I just rebased my branch and resolved the conflicts in the notary files.

@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #18659 (5cd456f) into main (982ff0a) will increase coverage by 22.60%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #18659       +/-   ##
===========================================
+ Coverage   44.78%   67.39%   +22.60%     
===========================================
  Files         235      980      +745     
  Lines       13067   106739    +93672     
  Branches     2665     2665               
===========================================
+ Hits         5852    71935    +66083     
- Misses       6920    30941    +24021     
- Partials      295     3863     +3568     
Flag Coverage Δ
unittests 67.39% <ø> (+22.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 750 files with indirect coverage changes

@MinerYang MinerYang added the release-note/update Update or Fix label May 25, 2023
Copy link
Contributor

@MinerYang MinerYang left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

LGTM

@MinerYang MinerYang merged commit 135ca37 into goharbor:main May 26, 2023
@OrlinVasilev
Copy link
Member

@malmor thank you for contributing :) congrats on your first PR here :)

@malmor malmor deleted the 18358-add-tls-1-3-support branch May 31, 2023 13:19
WilfredAlmeida pushed a commit to WilfredAlmeida/harbor that referenced this pull request Jul 8, 2023
Signed-off-by: malmor <[email protected]>
Co-authored-by: MinerYang <[email protected]>
Signed-off-by: Wilfred Almeida <[email protected]>
WilfredAlmeida pushed a commit to WilfredAlmeida/harbor that referenced this pull request Jul 8, 2023
@niko-la-petrovic
Copy link

For those who are looking to get TLSv1.3 before the release 2.9.0 comes out, you can add to line 61 of the prepare script

echo "Adding support for TLSv1.3"
sed -i $config_dir/nginx/nginx.conf -e 's/ssl_protocols TLSv1.2/ssl_protocols TLSv1.2 TLSv1.3/g'

Because the prepare script regenerates the nginx.conf, I think this is a sensible way to adjust the nginx.conf file in a reproducible way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for TLSv1.3 in nginx proxy
10 participants