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 Controller configuration to specify client CA #4664

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Mar 1, 2023

In the existing logic, Antrea Controller retrieves the client CA from ConfigMap kube-system/extension-apiserver-authentication, which is published by kube-apiserver. In some cases, the incoming connection is from the client with a certificate signed by a different CA bundle.

This patch adds a configuration in antrea-controller to allow user to specify the client CA. Antrea Controller will use it to validate the incoming client certificate in a mTLS connection.

@wenyingd wenyingd requested a review from tnqn March 1, 2023 02:16
@wenyingd wenyingd added this to the Antrea v1.12 release milestone Mar 27, 2023
@wenyingd wenyingd requested a review from jianjuns March 27, 2023 02:32
pkg/config/controller/config.go Outdated Show resolved Hide resolved
pkg/config/controller/config.go Outdated Show resolved Hide resolved
pkg/config/controller/config.go Outdated Show resolved Hide resolved
pkg/config/controller/config.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

Updated manifest to include the added configuration item.

pkg/config/controller/config.go Outdated Show resolved Hide resolved
pkg/config/controller/config.go Outdated Show resolved Hide resolved
pkg/config/controller/config.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #4664 (a5700d1) into main (6590c2a) will decrease coverage by 3.12%.
The diff coverage is 0.00%.

❗ Current head a5700d1 differs from pull request most recent head 2061b31. Consider uploading reports for the commit 2061b31 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4664      +/-   ##
==========================================
- Coverage   71.95%   68.83%   -3.12%     
==========================================
  Files         406      403       -3     
  Lines       60766    59781     -985     
==========================================
- Hits        43723    41153    -2570     
- Misses      14113    15782    +1669     
+ Partials     2930     2846      -84     
Flag Coverage Δ *Carryforward flag
e2e-tests 38.33% <ø> (-0.05%) ⬇️ Carriedforward from 3ecf6fd
integration-tests 34.41% <ø> (+0.63%) ⬆️
kind-e2e-tests 41.05% <ø> (-6.06%) ⬇️
unit-tests 59.86% <0.00%> (-2.76%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
cmd/antrea-controller/controller.go 0.00% <0.00%> (ø)

... and 95 files with indirect coverage changes

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

LGTM. @tnqn should review too.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, a comment on the configuration placement.

Comment on lines 77 to 80
apiserverAuthentication:
{{- with .Values.apiserverAuthentication }}
clientCAFile: {{ .clientCAFile | quote }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

I feel apiserverAuthentication is a too narrowed scope which can hardly group other configurations. It could just be apiserver or in the root group directly given apiserver is a generic component and not specific to one feature, and the other similar configurations tlsCipherSuites, tlsMinVersion are already in the root group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@wenyingd wenyingd force-pushed the support_client_ca branch 2 times, most recently from 86490e1 to 8fa141c Compare April 13, 2023 10:14
@@ -69,6 +69,9 @@ type ControllerConfig struct {
IPsecCSRSignerConfig IPsecCSRSignerConfig `yaml:"ipsecCSRSigner"`
// Multicluster configuration options.
Multicluster MulticlusterConfig `yaml:"multicluster,omitempty"`
// APIServerClientCAFile is the file path of the certificate bundle for all the signers that is recognized for incoming
// client certificates.
APIServerClientCAFile string `yaml:"apiserverClientCAFile,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Move it under TLSMinVersion which is also related to apiserver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -74,6 +74,10 @@ tlsCipherSuites: {{ .Values.tlsCipherSuites | quote }}
# TLS min version from: VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13.
tlsMinVersion: {{ .Values.tlsMinVersion | quote }}

# File path of the certificate bundle for all the signers that is recognized for incoming client
# certificates.
apiserverClientCAFile: {{ .Values.apiserverClientCAFile | quote }}
Copy link
Member

Choose a reason for hiding this comment

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

Like we don't name the other similar configurations apiserverTLSMinVersion, it could just be clientCAFile, I think there is no ambiguity that this is for client authentication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

Code LGTM but you need to make -C build/charts/ helm-docs

In the existing logic, Antrea Controller retrieves the client CA from ConfigMap
kube-system/extension-apiserver-authentication, which is published by kube-apiserver.
In some cases, the incoming connection is from the client with a certificate signed
by a different CA bundle.

This patch adds a configuration in antrea-controller to allow user to specify the client
CA. Antrea Controller will use it to validate the incoming client certificate in a mTLS
connection.

Signed-off-by: wenyingd <wenyingd@vmware.com>
@tnqn
Copy link
Member

tnqn commented Apr 14, 2023

/test-all

@tnqn
Copy link
Member

tnqn commented Apr 14, 2023

@jianjuns I suggested a minor update on the configuration placement. Please let us know if it makes sense to you.

@jianjuns
Copy link
Contributor

@jianjuns I suggested a minor update on the configuration placement. Please let us know if it makes sense to you.

Works for me.

@jianjuns jianjuns merged commit 38e849a into antrea-io:main Apr 14, 2023
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
In the current implementation, Antrea Controller retrieves the client CA from ConfigMap
kube-system/extension-apiserver-authentication, which is published by kube-apiserver.
In some use cases, the incoming connection is from a client with a certificate signed by
a different CA bundle.

This patch adds a configuration parameter for antrea-controller that allows users to
specify the client CA. Antrea Controller will use it to validate the incoming client
certificate in a TLS connection.

Signed-off-by: wenyingd <wenyingd@vmware.com>
@wenyingd wenyingd deleted the support_client_ca branch May 30, 2023 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants