Support providing a fixed public host key for SFTP uploads #6848
Conversation
| @@ -401,6 +402,12 @@ func (c *Controller) createInternalSupportBundleCollection(bundle *v1alpha1.Supp | |||
| Password: authentication.BasicAuthentication.Password, | |||
| } | |||
| } | |||
| // Validate the format of the public key here, to catch invalid keys early. | |||
| if bundle.Spec.FileServer.HostPublicKey != nil { | |||
There was a problem hiding this comment.
this reminds me we do also have a validate function. maybe that part need updates too.
There was a problem hiding this comment.
Done, thanks for pointing this out
There was a problem hiding this comment.
well i was trying to say PacketCapture also have a validate function in the controller.. but glad this also applied to supportbundle
There was a problem hiding this comment.
For packetcapture, I don't think we need to add extra validation for now (I wouldn't add a validation webhook just for that, but maybe others will disagree?). If the provided key is invalid, the upload will fail, just like it would fail if authentication data was missing or incorrect.
There was a problem hiding this comment.
i mean we already have a validatePacketCapture function to check protocol before, we can simply add this check here.
| @@ -560,6 +560,9 @@ type BundleFileServer struct { | |||
| // The URL of the bundle file server. It is set with format: scheme://host[:port][/path], | |||
| // e.g, https://api.example.com:8443/v1/supportbundles/. If scheme is not set, https is used by default. | |||
| URL string | |||
| // HostPublicKey specifies the only host public key that will be accepted when connecting to | |||
| // the file server. If omitted, any host key will be accepted, which is not recommended. | |||
| HostPublicKey []byte | |||
There was a problem hiding this comment.
I don't find this field being set. The conversion function should also be updated.
There was a problem hiding this comment.
It should be set when we copy the FileServer struct (https://github.com/antoninbas/antrea/blob/2043606f83e65bed9d20e0075e50fa8fed436726/pkg/controller/supportbundlecollection/controller.go#L554), unless I am misunderstanding your question.
I forgot to run code generation, but it is done now and the conversion function should have been updated (I don't think we have a manual one?).
There was a problem hiding this comment.
That code only sets the "internal" type, which will be converted into the "unversioned" API type in
antrea/pkg/controller/supportbundlecollection/store/collection.go
Lines 99 to 101 in 53d253a
The missing part is in the conversion between the internal type and the unversioned API type.
There was a problem hiding this comment.
Thanks, I added the missing code. I think I asked in the past, but I don't remember why we need 2 separate types here (unversioned API type and internal storage type)?
There was a problem hiding this comment.
When we implemented NetworkPolicy, we introduced the internal storage type to address the performance issue that it can't efficiently figure out whether a resource should be sent to a Node, and the pod members that should be sent to a Node, see
I guess SupportBundleCollection followed it for determining whether a resource should be sent to a Node. The unversioned type doesn't have a field to indicate that.
e182a5d to
2043606
Compare
|
I intend to have an e2e test for SupportBundleCollection, but I plan on doing it in a follow-up PR as this one is big enough already. There is some refactoring needed so we can share the SFTP server code between the SupportBundleCollection and PacketCapture e2e tests (not the case currently). |
2043606 to
0ca6053
Compare
|
/test-all |
0ca6053 to
85759a7
Compare
85759a7 to
e4d97fd
Compare
The SupportBundleCollection and PacketCapture CRDs can upload data to a user-provided SFTP server. Until now, there was no way for users to provide a verification mechanism for the host key from the server. Antrea would accept any host key when uploading the files, which is considered insecure. As a matter of fact, the usage of ssh.InsecureIgnoreHostKey as the verification callback was flagged in the code by security tools. And while there was a comment in the code explaining that "users can specify their own checks if needed", this is not accurate, as users have no way to provide a verification callback without changing the code manually. We are therefore introducing a new field for both the SupportBundleCollection and PacketCapture CRDs, so that users can provide a fixed host public key (as a base64-encoded string), which will be the only key accepted by Antrea for SFTP upload. This seems like a good start, and in the future we may expand that support so that users can provide a "known_hosts" OpenSSH file, either as a ConfigMap or Secret. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com> Generate manifests Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
e4d97fd to
6fcee11
Compare
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
6fcee11 to
37b18af
Compare
|
/test-all |
|
/test-vm-e2e |
2 similar comments
|
/test-vm-e2e |
|
/test-vm-e2e |
The SupportBundleCollection and PacketCapture CRDs can upload data to a user-provided SFTP server. Until now, there was no way for users to provide a verification mechanism for the host key from the server. Antrea would accept any host key when uploading the files, which is considered insecure. As a matter of fact, the usage of ssh.InsecureIgnoreHostKey as the verification callback was flagged in the code by security tools. And while there was a comment in the code explaining that "users can specify their own checks if needed", this is not accurate, as users have no way to provide a verification callback without changing the code manually.
We are therefore introducing a new field for both the SupportBundleCollection and PacketCapture CRDs, so that users can provide a fixed host public key (as a base64-encoded string), which will be the only key accepted by Antrea for SFTP upload. This seems like a good start, and in the future we may expand that support so that users can provide a "known_hosts" OpenSSH file, either as a ConfigMap or Secret.