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

feat: expose ssl_protocols from nginx configuration in harbor.yml #20637

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

malmor
Copy link
Contributor

@malmor malmor commented Jun 20, 2024

Thank you for contributing to Harbor!

Comprehensive Summary of your change

This PR adds ssl_protocols as a new configuration option to harbor.yml. This allows users to customize the nginx protocols - by default both TLSv1.2 and TLSv1.3 will be allowed, but one can restrict this to TLSv1.3 only.

There is no need to update the ssl_ciphers based on the configured ssl_protocols because nginx automatically disables unsupported ciphers. I tested the following combinations in a local nginx instance:

  • (Default) Neither ssl_protocols nor strong_ssl_ciphers configured - both protocols and all ciphers will be accepted
  • ssl_protocols unset, strong_ssl_ciphers enabled - both protocols will be accepted, with a restricted list of ciphers
  • ssl_protocols set to TLSv1.3, strong_ssl_ciphers disabled - only TLSv1.3 connections will be accepted
  • ssl_protocols set to TLSv1.3, strong_ssl_ciphers enabled - only TLSv1.3 connections will be accepted

Issue being fixed

Fixes #20627

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.
Signed-off-by: malmor <62105800+malmor@users.noreply.github.com>
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.22%. Comparing base (b7b8847) to head (2ee5dc6).
Report is 230 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #20637      +/-   ##
==========================================
- Coverage   67.56%   66.22%   -1.35%     
==========================================
  Files         991     1045      +54     
  Lines      109181   113476    +4295     
  Branches     2719     2845     +126     
==========================================
+ Hits        73768    75149    +1381     
- Misses      31449    34219    +2770     
- Partials     3964     4108     +144     
Flag Coverage Δ
unittests 66.22% <ø> (-1.35%) ⬇️

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

see 572 files with indirect coverage changes

@malmor
Copy link
Contributor Author

malmor commented Jun 20, 2024

The pipeline failures seem unrelated to the changes, not sure if I can fix them.

@MinerYang
Copy link
Contributor

MinerYang commented Jun 24, 2024

Hi @malmor ,

We appreciate your contribution and understood your requirements. However we would like to keep the harbor.yml file as simple as possible for most of the offline-installer users. So we'll hold this PR for a while to see if there's more requirements on this config.

As a workaround, you could change the config file manually then 'docker compose up -d' to meet the purpose.

Best,
Miner

@MinerYang MinerYang added the kind/requirement New feature or idea on top of harbor label Jun 24, 2024
@malmor
Copy link
Contributor Author

malmor commented Jun 24, 2024

Hey @MinerYang,
thank you for the response - I understand ����

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/requirement New feature or idea on top of harbor
5 participants