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

PWX-32188: TLS cipher-suite/version annotations #1153

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

zoxpx
Copy link
Collaborator

@zoxpx zoxpx commented Jul 19, 2023

  • adding annotations to select TLS version and cipher-suites for kube-controller-manager

What this PR does / why we need it:

Customers requested to fine-tune the TLS cipher-suites

FIX:

  • adding portworx.io/tls-cipher-suites annotation to select the TLS cipher suites
  • also adding portworx.io/tls-min-version annotation to select the minimal TLS version
  • right now, these add parameters to the portworx-pvc-controller-manager -- but we should extend it to all TLS-enabled services in PX deployment

Also note:

  • Golang's TLS-1.3 will ignore configured TLS ciphers
    • apparently this by design (also, lots of people complaining about this..)

Which issue(s) this PR fixes (optional)
Closes # PWX-32188

Special notes for your reviewer:

New annotations look like this:

kind: StorageCluster
metadata:
  annotations:
    portworx.io/tls-cipher-suites: TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
    portworx.io/tls-min-version: VersionTLS12

As a result, the parameters are added to the kube-controller-manager command:

  • kubectl get pod -Al name=portworx-pvc-controller
kind: Pod
spec:
  containers:
  - name: portworx-pvc-controller-manager
    image: registry.k8s.io/kube-controller-manager-amd64:v1.26.4
    command:
    - kube-controller-manager
    - --leader-elect=true
    - --controllers=persistentvolume-binder,persistentvolume-expander
    - --use-service-account-credentials=true
    - --leader-elect-resource-name=portworx-pvc-controller
    - --leader-elect-resource-namespace=portworx
    - --tls-min-version=VersionTLS12
    - --tls-cipher-suites=TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch coverage: 70.83% and project coverage change: -0.02 ⚠️

Comparison is base (ade43a1) 76.12% compared to head (e3bbb98) 76.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
- Coverage   76.12%   76.11%   -0.02%     
==========================================
  Files          64       64              
  Lines       17932    17980      +48     
==========================================
+ Hits        13651    13685      +34     
- Misses       3322     3328       +6     
- Partials      959      967       +8     
Impacted Files Coverage Δ
...rivers/storage/portworx/component/pvccontroller.go 91.50% <0.00%> (-2.13%) ⬇️
drivers/storage/portworx/util/util.go 72.20% <85.00%> (+0.77%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

}
req = strings.Trim(req, " \t")

if req == "VersionTLS10" || req == "VersionTLS11" || req == "VersionTLS12" || req == "VersionTLS13" {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Is this a case insensitive string? If yes, should we do a case insensitive match too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 06111b0

* adding annotations to select TLS version and cipher-suites for kube-controller-manager

Signed-off-by: Zoran Rajic <zox@portworx.com>
@zoxpx zoxpx force-pushed the PWX-32188_tls_annotations branch from 8219b8b to 06111b0 Compare July 19, 2023 22:17
@zoxpx
Copy link
Collaborator Author

zoxpx commented Jul 20, 2023

Thanks for the review Piyush -- merging this PR, cherry-picking for px-rel-23.7.0

@zoxpx zoxpx merged commit dd9ed40 into master Jul 20, 2023
6 of 8 checks passed
@zoxpx zoxpx deleted the PWX-32188_tls_annotations branch July 20, 2023 17:49
zoxpx added a commit that referenced this pull request Jul 21, 2023
* adding annotations to select TLS version and cipher-suites for kube-controller-manager

Signed-off-by: Zoran Rajic <zox@portworx.com>

Manualy fixed Conflicts:
	drivers/storage/portworx/util/util.go
	drivers/storage/portworx/util/util_test.go
nrevanna pushed a commit that referenced this pull request Aug 7, 2023
* adding annotations to select TLS version and cipher-suites for kube-controller-manager

Signed-off-by: Zoran Rajic <zox@portworx.com>
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.

None yet

2 participants