Support (m)TLS API Socket #24601

meln5674 · 2024-11-18T23:43:58Z

Add flags --tls-cert, --tls-key, --tls-ca/--tls-client-ca to the commands podman remote, podman system service, and podman system connection add to support serving the API socket using TLS and mTLS, as well as connecting to such a socket.

This relies on containers/common#2249 and will fail CI until merged.

Fixes #24583

Does this PR introduce a user-facing change?

* The `podman system service` command now supports serving over tcp with TLS and mTLS
* The `podman system connection add` command now supports creating connections to TLS and mTLS tcp sockets
* The `podman remote` commands now support connecting to TLS and mTLS tcp sockets

packit-as-a-service · 2024-11-18T23:49:22Z

Ephemeral COPR build failed. @containers/packit-build please check.

Luap99 · 2024-11-19T15:13:44Z

@jwhonce @baude @mheon FYI. Had you ever looked at TLS support for the podman service and remote client?
With the amount of code here this looks easy enough to accept to me but I was wondering if there is/was any reason why we do not support it.

mheon · 2024-11-19T16:18:49Z

I don’t think we ever looked into this. Largely because without certificate authentication - which is hard to do - TLS doesn’t add much. Other people not being any to read the traffic didn’t matter if we are exposing passwordless root over the network.

…

On Tue, Nov 19, 2024 at 10:14 Paul Holzinger ***@***.***> wrote: @jwhonce <https://github.com/jwhonce> @baude <https://github.com/baude> @mheon <https://github.com/mheon> FYI. Had you ever looked at TLS support for the podman service and remote client? With the amount of code here this looks easy enough to accept to me but I was wondering if there is/was any reason why we do not support it. — Reply to this email directly, view it on GitHub <#24601 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCFVIQJ3XD2SIG42VKL2BNIT5AVCNFSM6AAAAABSA2GWC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBVHE4TGNBQGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mheon · 2024-11-19T18:21:08Z

Just skimmed, but LGTM on approach. A lot less code than I was expecting for certificate auth.

Now, if we have to start providing docs for how to do certificate auth right, I think the amount of changes grows substantially, but I don't think it's really worth it at this point.

TomSweeneyRedHat · 2024-11-21T20:53:02Z

Sending the directory location of the private key out over the wire doesn't give me the warm fuzzies. Is it something that other software does?

jwhonce · 2024-11-21T21:16:36Z

@Luap99 I seem to remember the idea behind using the ssh executable as well as the golang libraries was to push this kind of handshake into that code vs. podman. But @cdoern did that work, so I don't know where it stands.

meln5674 · 2024-11-22T16:45:13Z

Sending the directory location of the private key out over the wire doesn't give me the warm fuzzies. Is it something that other software does?

That certainly wasn't my intent, but I'm not sure what you're referring to. Is that a consequence of adding it to the conf file that I'm unaware of?

Luap99 · 2024-11-25T12:35:24Z

Sending the directory location of the private key out over the wire doesn't give me the warm fuzzies. Is it something that other software does?

That certainly wasn't my intent, but I'm not sure what you're referring to. Is that a consequence of adding it to the conf file that I'm unaware of?

It is not being send anywhere, both the client and server read the files locally and then use the certificates to perform a normal TLS handshake via the go std lib AFAICT.
@TomSweeneyRedHat What are you referring to by sending the private key over the write?

@Luap99 I seem to remember the idea behind using the ssh executable as well as the golang libraries was to push this kind of handshake into that code vs. podman. But @cdoern did that work, so I don't know where it stands.

@jwhonce I am not sure what are you referring to? The question for me was if there was a specific reason why TLS support was not done before. If not then I think we should accept this PR

Luap99

Also this will need some e2e or system tests where we spawn a server with TLS and the connect with the remove client.

Luap99 · 2024-11-25T12:41:14Z

pkg/util/tls.go

+	"os"
+)
+
+func ReadCertBundle(path string) (*x509.CertPool, error) {


can you mope this in a new separate package, maybe call it cert and then name the function ReadBundle()

We should really move away from dumping everything into util packages as this causes a lot of unwanted side effects via other imports that really should not have to be imported on the remote client, i.e. #23818. Fixing that will be quite some work but in the meantime we should not add new things there that are needed by the remote client.

Luap99 · 2024-11-25T12:44:16Z

pkg/bindings/connection.go

+	if len(tlsCAFile) != 0 || len(tlsCertFile) != 0 || len(tlsKeyFile) != 0 {
+		logrus.Debugf("using TLS cert=%s key=%s ca=%s", tlsCertFile, tlsKeyFile, tlsCAFile)
+		transport.TLSClientConfig = &tls.Config{}
+		connection.tls = true
+	}
+	if len(tlsCAFile) != 0 {
+		pool, err := util.ReadCertBundle(tlsCAFile)
+		if err != nil {
+			return connection, fmt.Errorf("unable to read CA bundle: %w", err)
+		}
+		transport.TLSClientConfig.RootCAs = pool
+	}
+	if len(tlsCertFile) != 0 && len(tlsKeyFile) != 0 {


The condition here do not seem to line up perfectly.

If only tlsCertFile is set then the first condition matches and says tls is used but then we never add the the key on the last one. That case should return a hard error I would assume.

My intent was to have that check performed in the cmd/podman/... packages, but on reflection it makes sense to also perform the same check here, as that wouldn't account for the env vars. Done.

Luap99 · 2024-11-25T12:47:57Z

cmd/podman/system/connection/add.go

+		if cmd.Flags().Changed("tls-cert") {
+			return errors.New("--tls-cert option not supported for ssh scheme")
+		}
+		if cmd.Flags().Changed("tls-key") {
+			return errors.New("--tls-key option not supported for ssh scheme")
+		}
+		if cmd.Flags().Changed("tls-ca") {
+			return errors.New("--tls-ca option not supported for ssh scheme")
+		}
 		return ssh.Create(entities, sshMode)
 	case "unix":
 		if cmd.Flags().Changed("identity") {
 			return errors.New("--identity option not supported for unix scheme")
 		}
+		if cmd.Flags().Changed("tls-cert") {
+			return errors.New("--tls-cert option not supported for unix scheme")
+		}
+		if cmd.Flags().Changed("tls-key") {
+			return errors.New("--tls-key option not supported for unix scheme")
+		}
+		if cmd.Flags().Changed("tls-ca") {
+			return errors.New("--tls-ca option not supported for unix scheme")
+		}


This duplicates the same conditions. Move them outside the switch case and match if scheme != tcp so you do not duplicate that

…terial for TCP remotes Signed-off-by: Andrew Melnick <[email protected]>

* Moved cert bundle read to separate package * Removed duplication for non-tcp tls flag check * Added tls info to `system connection list` * Removed TCP warning if TLS is enabled * Fixed not using TLS when using ABI instead of remote * Added central check for cert without key or vice-versa Signed-off-by: Andrew Melnick <[email protected]>

meln5674 · 2024-11-30T23:45:27Z

Also this will need some e2e or system tests where we spawn a server with TLS and the connect with the remove client.

Of course. I was hoping to have my environment capable of passing tests on main before I went on vacation last week, but that didn't end up happening. After throwing up my hands and just making a fresh CentOS VM, I have that working now.

I'd like to use any existing tests for SSH/TCP remotes as a basis for my tests, as well as ensure my tests have parity with them, but for the life of me, I cannot seem to find any such tests. Would you mind pointing me to where they are?

TomSweeneyRedHat · 2024-12-09T21:34:49Z

@Luap99 sorry, just back to this. This line: https://github.com/containers/podman/pull/24601/files#diff-cf5cc76e70a369c5221f08f29ec0452710af1f4ecd423c162d5b669688416caeR525 seemed to indicate that you were looking for the directory where the private key resigned. Unless I misinterpreted, that doesn't feel right.

meln5674 · 2024-12-09T23:56:45Z

@Luap99 sorry, just back to this. This line: https://github.com/containers/podman/pull/24601/files#diff-cf5cc76e70a369c5221f08f29ec0452710af1f4ecd423c162d5b669688416caeR525 seemed to indicate that you were looking for the directory where the private key resigned. Unless I misinterpreted, that doesn't feel right.

Yes, the purpose of this line is to create a command line argument so that the user can provide the path to their TLS client private key, which is necessary to perform authentication. Unless I too have misinterpreted something else in the codebase, neither that path, nor especially not the data in that file, ever goes over the wire. This is the analog of the --identity flag for SSH authentication.

* Fixed attach endpoint client using unencrypted socket when TLS is enabled * Duplicated libpod remote unix socket e2e suite for plaintext TCP, TLS, and mTLS Signed-off-by: Andrew Melnick <[email protected]>

openshift-ci · 2024-12-30T17:28:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: meln5674
Once this PR has been reviewed and has the lgtm label, please assign luap99 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

meln5674 · 2024-12-30T17:50:30Z

Also this will need some e2e or system tests where we spawn a server with TLS and the connect with the remove client.

Apologies for the delay on this, a number of other items were pushed onto my priority stack, but I've been able to resume work on it.

As mentioned, there weren't any substantial e2e tests for the TCP API, and the libpod remote test suite was entirely using the unix socket, so I have refactored it to allow running it for unix, plain TCP, TLS, and mTLS. This might seem excessive, but it was only by running this complete suite did I find additional issues I had to fix. Suggestions are welcome for how to achieve similar coverage with fewer tests.

Speaking of said issues, it would seem that the container attach client has a subtle race condition in it, as well as a memory leak.

By overwriting the http transport and capturing the net.Conn as a local variable, if the same Connection is used concurrently, there is a possibility that the first net.Conn will be overwritten on the first attach call's local variable, resulting in the two calls proxying each-other's streams, or just outright corrupting them.

As well, because the original transport is never restored, if the same client is used to attach to multiple containers, the closure is never freed, meaning the dial function will call an ever-increasing stack of closures. Fixing this issue felt outside the scope of this PR, but I have added a note warning of it to whoever feels like addressing it in the future.

I would suggest adding some sort of DoRequestWithConn method which handles using the new transport for that request alone and manually performs the dialing, writing the request, and reading the response, and validating the upgrade.

Finally, there were a number of other e2e tests that were failing in my fresh centos 9 environment, even on main, so I have simply ignored them for the time being.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Nov 18, 2024

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Nov 18, 2024

meln5674 mentioned this pull request Nov 18, 2024

Add fields for TLS material to destination config containers/common#2249

Draft

Luap99 reviewed Nov 25, 2024

View reviewed changes

meln5674 added 2 commits November 30, 2024 13:35

Add flags to "system service" and "system connnection add" for TLS ma…

0259e04

…terial for TCP remotes Signed-off-by: Andrew Melnick <[email protected]>

meln5674 force-pushed the feature/remote-tls branch from cceab7f to 0748cfe Compare November 30, 2024 23:03

mheon mentioned this pull request Dec 9, 2024

docs: add example system service tcp #24782

Open

Continue work on TLS API Support

46db840

* Fixed attach endpoint client using unencrypted socket when TLS is enabled * Duplicated libpod remote unix socket e2e suite for plaintext TCP, TLS, and mTLS Signed-off-by: Andrew Melnick <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support (m)TLS API Socket #24601

Support (m)TLS API Socket #24601

meln5674 commented Nov 18, 2024 •

edited

Loading

packit-as-a-service bot commented Nov 18, 2024

Luap99 commented Nov 19, 2024

mheon commented Nov 19, 2024 via email

mheon commented Nov 19, 2024

TomSweeneyRedHat commented Nov 21, 2024

jwhonce commented Nov 21, 2024

meln5674 commented Nov 22, 2024

Luap99 commented Nov 25, 2024

Luap99 left a comment

Luap99 Nov 25, 2024

meln5674 Nov 30, 2024

Luap99 Nov 25, 2024

meln5674 Nov 30, 2024

Luap99 Nov 25, 2024

meln5674 Nov 30, 2024

meln5674 commented Nov 30, 2024

TomSweeneyRedHat commented Dec 9, 2024

meln5674 commented Dec 9, 2024

openshift-ci bot commented Dec 30, 2024

meln5674 commented Dec 30, 2024

Support (m)TLS API Socket #24601

Are you sure you want to change the base?

Support (m)TLS API Socket #24601

Conversation

meln5674 commented Nov 18, 2024 • edited Loading

Does this PR introduce a user-facing change?

packit-as-a-service bot commented Nov 18, 2024

Luap99 commented Nov 19, 2024

mheon commented Nov 19, 2024 via email

mheon commented Nov 19, 2024

TomSweeneyRedHat commented Nov 21, 2024

jwhonce commented Nov 21, 2024

meln5674 commented Nov 22, 2024

Luap99 commented Nov 25, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Nov 25, 2024

Choose a reason for hiding this comment

meln5674 Nov 30, 2024

Choose a reason for hiding this comment

Luap99 Nov 25, 2024

Choose a reason for hiding this comment

meln5674 Nov 30, 2024

Choose a reason for hiding this comment

Luap99 Nov 25, 2024

Choose a reason for hiding this comment

meln5674 Nov 30, 2024

Choose a reason for hiding this comment

meln5674 commented Nov 30, 2024

TomSweeneyRedHat commented Dec 9, 2024

meln5674 commented Dec 9, 2024

openshift-ci bot commented Dec 30, 2024

meln5674 commented Dec 30, 2024

meln5674 commented Nov 18, 2024 •

edited

Loading