Add support for PSK to remote CIB and deprecate insecure methods#4051
Add support for PSK to remote CIB and deprecate insecure methods#4051clumens wants to merge 15 commits intoClusterLabs:mainfrom
Conversation
|
CI failure is unrelated to this PR: Let's see if #4052 fixes it. |
clumens
force-pushed
the
remote-cib-psk
branch
from
600865c to
834fb83
Compare
|
Rebased on main to pick up that logging fix so we can get the green check. |
clumens
force-pushed
the
remote-cib-psk
branch
2 times, most recently
from
37ff190 to
947f634
Compare
daemons/based/based_remote.c
Outdated
| * connections, all clients will have the same username so we don't need | ||
| * to look it up anywhere. | ||
| */ | ||
| if (!pcmk__str_eq(CRM_DAEMON_USER, username, pcmk__str_none)) { |
There was a problem hiding this comment.
The CIB manager doesn't require this (see cib_remote_auth()). This caused me a bit of confusion. But I guess, counterintuitively, this is unrelated to the username and password used for authentication to the CIB manager. The TLS credentials are set using CRM_DAEMON_USER in this same commit.
I'm trying to think of any reason why that might be a problem when the client requests to use a different user (via the CIB_user environment variable). I don't have any specific complaint at the moment.
I don't think it matters -- CRM_DAEMON_USER is used at the GnuTLS level, and CIB_user is used at the CIB manager level, without the two interacting.
There was a problem hiding this comment.
In general, our authentication is confusing and in the interests of trying to keep things as similar within pacemaker as possible, I might have also made things more confusing.
You're right, there are two layers of authentication, and everything I've been doing here should be at the gnutls layer. What I've set up here does mean that the only valid username is CRM_DAEMON_USER, but that is in line with how the Pacemaker Remote node authentication works - the only valid username there is DEFAULT_REMOTE_USERNAME.
Ideally we'd use the username/key system that gnutls wants us to use, but having different systems for different portions of pacemaker seemed more confusing than just using the same goofy system everywhere.
Checking the username is kind of pointless, but it just seems odd to me to have authentication code that doesn't check it.
There was a problem hiding this comment.
I might have also made things more confusing.
My initial answer was along the lines of "Heheh, I don't think so. That'd be tough to do. Besides, if we had different keys for different usernames, we'd have to check those on the server side."
Thinking a little bit more, I'm on the fence. We could use the gnutls credential file scheme and avoid needing a callback function. That way we could support different keys for different CIB_users.
I'm trying to think of whether this would make a difference for anyone. I'm starting to think that it could.
- On the one hand, maybe a cluster has multiple admin users. It could make sense to give each of them their own credential file (with a PSK) on the remote admin machine, so that they can only log in as themselves -- especially if they have different ACLs or something.
- On the other hand, each of them already has to enter the
CIB_user's password to complete the signon. - Back to the first hand:
- The different credential files could act like a sort of 2FA, so that the remote admin user needs both the correct PSK in the credentials file and the correct password. (But: we give precedence to X509 for whatever reason, and I'm not sure off the top of my head whether our X509 scheme has an analogue where each user has their own cert.)
- Couldn't this eliminate the need for entering the
CIB_userpassword altogether? I'm wondering if we could allow passwordless authentication when PSK and/or X509 is configured for a particular user. It doesn't seem like it would be grossly insecure -- as an analogy, I'm thinking of copying SSH keys, which is common practice.
You've made an argument for consistency with remote node authentication. That's a reasonable argument, as consistency tends to make things much easier to, um, reason about. However, IIRC there is no real place for another username in remote node authentication. All of that stuff is done within Pacemaker -- almost as an implementation detail but with the administrator needing to provide a credentials file when setting up the cluster. The remote CIB administration stuff has a logical place for different user accounts.
There was a problem hiding this comment.
* On the one hand, maybe a cluster has multiple admin users. It could make sense to give each of them their own credential file (with a PSK) on the remote admin machine, so that they can only log in as themselves -- especially if they have different ACLs or something.
Yes, this does make sense.
* The different credential files could act like a sort of 2FA, so that the remote admin user needs both the correct PSK in the credentials file and the correct password. (But: we give precedence to X509 for whatever reason, and I'm not sure off the top of my head whether our X509 scheme has an analogue where each user has their own cert.)
I have to re-learn how all the certificate stuff works every time I need to look at it just because it's so complicated and different from anything else I ever look at. So with that in mind... at the moment, each client can have its own X509 cert. A "client" would typically be a system that an admin would connect from, so not technically exactly the same as a per-user cert, but pretty close and maybe close enough that the distinction doesn't matter.
They still have to login with a username and password. I'm not sure whether removing that layer is a good idea or not.
* Couldn't this eliminate the need for entering the `CIB_user` password altogether? I'm wondering if we could allow passwordless authentication when PSK and/or X509 is configured for a particular user. It doesn't seem like it would be grossly insecure -- as an analogy, I'm thinking of copying SSH keys, which is common practice.
Yes, definitely. That was something I thought of earlier too. And if we could justify getting rid of the user/password combination with certs (which again, not sure that's safe) then I think we could eliminate all of the PAM related code.
You've made an argument for consistency with remote node authentication. That's a reasonable argument, as consistency tends to make things much easier to, um, reason about. However, IIRC there is no real place for another username in remote node authentication. All of that stuff is done within Pacemaker -- almost as an implementation detail but with the administrator needing to provide a credentials file when setting up the cluster. The remote CIB administration stuff has a logical place for different user accounts.
I agree, and another thing to think about when it comes to the remote node authentication is that the client/server model there is flipped. The remote node is the server, and the cluster is the client. Having multiple usernames there makes less sense.
I think it's worth looking into this and trying to get it done in the next couple days. I would definitely like to be more aligned with how gnutls wants us to work, and I like the idea of being able to deprecate CIB_user/CIB_passwd.
nrwahl2
left a comment
nrwahl2
left a comment
There was a problem hiding this comment.
I left some substantial thoughts in #4051 (comment). If we want to reconsider the approach, we can do that either now or later. Feel free to merge this and get the basic feature in place, if you would like.
To me, pcmk__tls_add_psk_key is not especially obviously named. This function should only be called on TLS clients - it's not some general purpose PSK key management function. It has no purpose on the server side of a connection. Therefore, rename it and update the doxygen to be clearer. Ref T961
…ername. Pre-shared keys in gnutls are tied to a specific username. See the man page for gnutls_psk_set_client_credentials or https://www.gnutls.org/manual/html_node/PSK-credentials.html for more details. Right now, we only support PSK for remote node authentication, and we only support a single username for that. We'll be adding PSK support to remote CIB authentication in a later patch, and that will allow specifying other usernames. Ref T961
gnutls expects that for PSK authentication, there will be a password file somewhere with the format "username:key". The server tells gnutls where that password file is, and then when a client connects, it gives the server a username and key. gnutls reads that file and looks for a match. execd doesn't do that. For Pacemaker Remote node connections, every client always has the same username (DEFAULT_REMOTE_USERNAME, or "lrmd") and there's only one key. That's because the key is just a single file that's referenced in /etc/sysconfig/pacemaker. In order for us to make gnutls work the way we want, we have to register a callback function. This is probably okay because in the remote node world, the cluster is the client and the remote node is the server. It makes some sense that each remote node would have the same username. This callback function will get called every time a client attempts to connect. It will load the key from disk (which, because we only support a single key, it's probably been cached - I think this is why we even have the caching layer for keys in the first place) into a variable, and then gnutls will check its validity. We additionally try to load the key from disk outside of gnutls's control as a way to log an error if it's not present, and I suppose as a way to have it cached for the first connection. Note one behavioral change: previously, our callback function wasn't even checking the username. This is somewhat pointless since the username will always be the same, but it still seems worth doing as a best practice. Because all of the above only applies to execd (PSK support for remote CIB operations will not work this way), just get rid of pcmk__tls_add_psk_callback and inline its contents in execd. Ref T961
This is the server side of things. The way this works is that the server has a file where each line is "username:key". If this file is present and useable, PSK authentication will be supported. If it's just present but not useable, we assume the admin made a mistake in configuration and error out. Otherwise, we'll fall back to anonymous authentication for now. gnutls will handle reading this file, looking for a user match, and checking the key. This patch is made more complicated by refusing to use a credentials file with the wrong ownerships or permissions. It's annoying, but that's the way a lot of security software works and I think it's probably a best practice. Note that the location of the credentials file is not configurable. Ref T961
This function can be used to add two kinds of PSK keys: (1) Binary (or raw, as gnutls calls them) keys. This is just a binary blob of data and is what Pacemaker Remote keys tend to be. Various documents online describe using dd to create the authentication key for remote nodes, and I think pcs does something similar. (2) Text files with name:key rows. Here, the key is a string of hex characters. This is what remote CIB administration will use. gnutls needs to be told what format the key is in when it's being loaded. I feel like this is opening us up to some pretty annoying bugs, but I don't see a way around it if we're to support both kinds of things we want to do. In particular, I'm worried that someone will be using a text-based Pacemaker Remote key. Ref T961
This is for the exact same reasons as pcmk__tls_client_add_psk_key. Hex keys won't work with gnutls unless the trailing newline is stripped from the read string. Ref T961
This is the client side of things. For the client, we need to get the username and key from somewhere. I've chosen to use $CIB_user and to overload $CIB_key_file (which is already in use for X509 cert support) for this purpose. Overloading the latter should be fine - there's nowhere we check just that environment variable to decide on cert support, and you can't use both certs and PSK at the same time. Given those two environment variables, all we have to do is load the key and pass it to gnutls which takes care of the rest. Again, this patch is made more complicated by refusing to use a credentials file with the wrong ownerships or permissions Ref T961
…ey_location. Ref T961
No test output was affected. Ref T961
This CIB property is deprecated in favor of remote-tls-port which is more secure and has been supported since at least 2014. For now, it will still be recognized but will log a warning. Also mark it as deprecated in the docs and remove references to it being something that you can use. Ref T961
This is an insecure authentication method where we use an encrypted TLS channel for communication, but there's no authentication on the channel beyond that. Well, that's not completely true - for remote CIB operations, you do still need a username and password. However, the username is always the same so that's easy for people to figure out. Instead, people should be using X509 certificates or PSK. A future release will remove support for anonymous authentication and require use of one of those other mechanisms. Fixes T961
|
@nrwahl2 The patches here that are identical to the first version are:
|
2 participants
No description provided.