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.

Sign up for GitHub

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

Jump to bottom

http2 enabled and ciphers changed to get an A+ rating instead of B fr… #16990

Merged
merged 8 commits into from
May 29, 2023
Merged

http2 enabled and ciphers changed to get an A+ rating instead of B fr… #16990

merged 8 commits into from
May 29, 2023

Conversation

mcsage
Copy link
Contributor

@mcsage mcsage commented Jun 13, 2022

http2 enabled and ciphers changed to get an A+ rating instead of B from ssllabs

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Issue being fixed

Fixes #(issue)

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.

@mcsage mcsage requested a review from a team as a code owner June 13, 2022 08:53
@zyyw
Copy link
Contributor

zyyw commented Jun 20, 2022

We may have concerns during ssl handshake phase if the cipher changed.

Could you please make this config as optional in addition to the default behavior as it is?

@github-actions
Copy link

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

@github-actions github-actions bot added the Stale label Sep 30, 2022
@wy65701436 wy65701436 added the help wanted The issues that is valid but needs help from community label Oct 17, 2022
@github-actions github-actions bot removed the Stale label Oct 19, 2022
@OrlinVasilev
Copy link
Member

https://www.ssllabs.com/ssltest/analyze.html?d=demo.goharbor.io

@github-actions
Copy link

github-actions bot commented Jan 7, 2023

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

@github-actions github-actions bot added the Stale label Jan 7, 2023
@OrlinVasilev OrlinVasilev removed the Stale label Jan 9, 2023
@OrlinVasilev
Copy link
Member

@zyyw @wy65701436 @mcsage is that still valid ?

@OrlinVasilev OrlinVasilev added the release-note/update Update or Fix label Jan 9, 2023
Make strong cipher cfg optional
a94fe7d 
Signed-off-by: Stephan Hohn <[email protected]>
@mcsage mcsage force-pushed the enable_http2_and_use_more_secure_ciphers branch from 44bc1a3 to a94fe7d Compare February 16, 2023 20:23
Stephan Hohn added 2 commits February 16, 2023 21:26
Make strong cipher cfg optional
27fec4c 
Signed-off-by: Stephan Hohn <[email protected]>
Make strong cipher cfg optional
9fd2620 
Signed-off-by: Stephan Hohn <[email protected]>
@mcsage
Copy link
Contributor Author

mcsage commented Feb 16, 2023

I think it's still valid. I made it optional.

@wy65701436
Copy link
Contributor

@YangJiao0817 please help to test it, cc @stonezdj

@wy65701436
Copy link
Contributor

@zyyw @MinerYang Should this be considered a Helm impact? If so, I would prefer to include it in version 2.9.

@wy65701436 wy65701436 assigned zyyw and stonezdj and unassigned OrlinVasilev Mar 13, 2023
@codecov
Copy link

codecov bot commented Mar 13, 2023 •
edited
Loading

Codecov Report

Merging #16990 (bed2808) into main (11d6bb4) will increase coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16990      +/-   ##
==========================================
+ Coverage   67.24%   67.38%   +0.13%     
==========================================
  Files         980      980              
  Lines      106749   106749              
  Branches     2665     2665              
==========================================
+ Hits        71787    71932     +145     
+ Misses      31112    30948     -164     
- Partials     3850     3869      +19
Flag Coverage Δ
unittests 67.38% <ø> (+0.13%) ⬆️

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

see 10 files with indirect coverage changes

@stonezdj
Copy link
Contributor

Link to issue #16367

@stonezdj stonezdj self-requested a review March 15, 2023 09:00
stonezdj
stonezdj approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@stonezdj stonezdj left a comment

Choose a reason for hiding this comment

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

LGTM

wy65701436
wy65701436 approved these changes Apr 3, 2023
View reviewed changes
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
Copy link
Contributor

We probably need consider the changes in the nginx /nginx.https.conf.jinja not just portal/nginx.conf.jinja

@MinerYang MinerYang self-assigned this Apr 13, 2023
@MinerYang
Copy link
Contributor

Will submit another follow-up PR to add in nginx /nginx.https.conf.jinja as well as take care of the prepare migration part

@MinerYang MinerYang merged commit 4f3393e into goharbor:main May 29, 2023
@OrlinVasilev
Copy link
Member

@mcsage great to see that merged :) congrats :) and welcome to the club!

WilfredAlmeida pushed a commit to WilfredAlmeida/harbor that referenced this pull request Jul 8, 2023
@mcsage @wy65701436
@MinerYang @WilfredAlmeida
http2 enabled and ciphers changed to get an A+ rating instead of B fr… (
1199d9a 
goharbor#16990)

* Make strong cipher cfg optional
Signed-off-by: Stephan Hohn <[email protected]>

---------

Signed-off-by: Stephan Hohn <[email protected]>
Signed-off-by: MinerYang <[email protected]>
Co-authored-by: Stephan Hohn <[email protected]>
Co-authored-by: Wang Yan <[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
@mcsage @wy65701436
@MinerYang @WilfredAlmeida
http2 enabled and ciphers changed to get an A+ rating instead of B fr… (
20f8e97 
goharbor#16990)

* Make strong cipher cfg optional
Signed-off-by: Stephan Hohn <[email protected]>

---------

Signed-off-by: Stephan Hohn <[email protected]>
Signed-off-by: MinerYang <[email protected]>
Co-authored-by: Stephan Hohn <[email protected]>
Co-authored-by: Wang Yan <[email protected]>
Co-authored-by: MinerYang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stonezdj stonezdj stonezdj approved these changes

@wy65701436 wy65701436 wy65701436 approved these changes

Assignees

@stonezdj stonezdj

@wy65701436 wy65701436

@zyyw zyyw

@MinerYang MinerYang

Labels
help wanted The issues that is valid but needs help from community pending-for-more-info release-note/update Update or Fix target/2.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mcsage @zyyw @OrlinVasilev @wy65701436 @stonezdj @MinerYang