Allison is looking into this, to help out the sssd team. It's not that trivial.

(In reply to Martin Pitt from comment #3)
> Allison is looking into this, to help out the sssd team. It's not that
> trivial.

Hi Martin,

please let me know if this is really urgent for cockpit, in this case I can prepare a patch if we agree on the interface. I wonder if a org.freedesktop.sssd.infopipe.Users.FindByCertificateIfVaild() interface would be more easy for your use case?

bye,
Sumit

It's not "really urgent", we have a few months to fix this (as we just moved past the 8.5/9.0beta freeze anyway). So let's make sure we have a good design.

Allison and I discussed that a little bit last week. Some notes:

 - We discussed letting cockpit-tls do the actual CA/CRL checks (through GnuTLS API), and only let sssd tell us the cert paths (i.e. parse its configuration). This does not work because (1) cockpit-tls gets involved both too early and also too often, and (2) is not running as root.
 - So we went back to the original proposal of doing the call in cockpit-session (which does not use GnuTLS, and as suid root program we also want to avoid that), and calling out to infopipe.
 - infopipe is single-threaded, so must not block for too long. Hence this new API should *not* do OCSP. It's questionable whether any customer wants to have OCSP in the first place (my gut feeling is "no"), and it is also a big privacy problem if it's an external provider. So for now, *if* OCSP is enabled, let's just return that fact through an extra dict flag in the return value.
 - So a FindByCertificateIfValid(cert: s) -> (username: s, info: a{sv}) API should work well for us; in the initial implementation, the returned info dict can just be empty, but it's a place to inform about OCSP or other steps to be done in the future. However, it is not clear to me yet whether this can be implemented in sssd in a straightforward way.
 - Having a separate ValidateCertificate() which just does the validation, but not the mapping, would work equally well, and perhaps make the API nicer and more orthogonal.

I suggest Allison and you discuss the details directly.

Thanks!

The main issue is that online (OCSP) checks (as done by the pam module, for example) involve IO and potentially long waits.  In the pam module those are done from the pkcs11 helper, which can block for as long as it wants to (and for which there is a timeout).  If we do that with OpenSSL, then we have the choice between a blocking (forever) call or a "non-blocking" variant which presents an API which might theoretically be possible to wrap in a mainloop, but with a lot of work. It seems like this is probably a bad fit for the infopipe service, which is where we started to get the idea of the client becoming responsible for performing some of the checks.

I do think (due to the architecture of cockpit: our point of contact with SSSD is from a place where we have no crypto library) that the CA and CRL checks absolutely should be performed inside of SSSD.

And maybe also OCSP: that's gonna be a bit of research, though.

(In reply to Martin Pitt from comment #5)
> It's not "really urgent", we have a few months to fix this (as we just moved
> past the 8.5/9.0beta freeze anyway). So let's make sure we have a good
> design.
> 
> Allison and I discussed that a little bit last week. Some notes:
> 
>  - We discussed letting cockpit-tls do the actual CA/CRL checks (through
> GnuTLS API), and only let sssd tell us the cert paths (i.e. parse its
> configuration). This does not work because (1) cockpit-tls gets involved
> both too early and also too often, and (2) is not running as root.

Hi,

you do not have to be root to call SSSD via DBus, as long as the user is added to the 'allowed_uids' in sssd.conf.

>  - So we went back to the original proposal of doing the call in
> cockpit-session (which does not use GnuTLS, and as suid root program we also
> want to avoid that), and calling out to infopipe.
>  - infopipe is single-threaded, so must not block for too long. Hence this
> new API should *not* do OCSP. It's questionable whether any customer wants
> to have OCSP in the first place (my gut feeling is "no"), and it is also a
> big privacy problem if it's an external provider. So for now, *if* OCSP is
> enabled, let's just return that fact through an extra dict flag in the
> return value.

The validation wouldn't be done inside of the infopipe process, but by the p11_child helper process. So delays during OCSP are not an issue. OCSP is enabled by default if the certificate has the related extension.


>  - So a FindByCertificateIfValid(cert: s) -> (username: s, info: a{sv}) API
> should work well for us; in the initial implementation, the returned info
> dict can just be empty, but it's a place to inform about OCSP or other steps
> to be done in the future. However, it is not clear to me yet whether this
> can be implemented in sssd in a straightforward way.

Since SSSD can handle OCSP I think there is no need for any additional information.

>  - Having a separate ValidateCertificate() which just does the validation,
> but not the mapping, would work equally well, and perhaps make the API nicer
> and more orthogonal.

Whatever works best for you.

bye,
Sumit

> 
> I suggest Allison and you discuss the details directly.
> 
> Thanks!

(In reply to Allison Karlitskaya from comment #6)
> The main issue is that online (OCSP) checks (as done by the pam module, for
> example) involve IO and potentially long waits.  In the pam module those are
> done from the pkcs11 helper, which can block for as long as it wants to (and
> for which there is a timeout).  If we do that with OpenSSL, then we have the
> choice between a blocking (forever) call or a "non-blocking" variant which
> presents an API which might theoretically be possible to wrap in a mainloop,
> but with a lot of work. It seems like this is probably a bad fit for the
> infopipe service, which is where we started to get the idea of the client
> becoming responsible for performing some of the checks.
> 
> I do think (due to the architecture of cockpit: our point of contact with
> SSSD is from a place where we have no crypto library) that the CA and CRL
> checks absolutely should be performed inside of SSSD.
> 
> And maybe also OCSP: that's gonna be a bit of research, though.

Hi,

see my reply to Martin's questions, doing OCSP in SSSD is not an issue, it's enabled by default.

HTH

bye,
Sumit

> The validation wouldn't be done inside of the infopipe process, but by the p11_child helper process.

And infopipe handles this asynchronously, so that it doesn't block until the p11_child process finishes? My concern is mostly that infopipe (like all D-Bus servers) must be able to handle other method calls in parallel, even while a long-running one is being handled asynchronously. To clarify, it's *not* a problem if calling FindByCertificateIfValid() takes some time; it is a problem if during that time nothing else can talk to the infopipe service as it's blocked on that method call. This needs to be done with async programming or threads.

(In reply to Sumit Bose from comment #7)
> The validation wouldn't be done inside of the infopipe process, but by the
> p11_child helper process. So delays during OCSP are not an issue. OCSP is
> enabled by default if the certificate has the related extension.

My understanding from having read the code here is that this process exists to perform a certificate validation against the smartcard via PKCS11 and return the result of that to the PAM module.  In the cockpit case, we don't have the smartcard connected to the machine, but rather receive it as a browser client certificate (which we validate ourselves).  In order to use p11_child as part of this process, we'd need to have a way to tell the PAM module "we've already validated that the user is in possession of the private key for this certificate", and pass the certificate itself in.

That's not currently supported in p11_child, as far as I understand it, but it would be a fascinating idea, because what we're also trying to do is to start a PAM session...

Otherwise, it's going to look like one of:

 1) copy the logic from the PAM module to the infopipe, and make a note to keep the two copies in sync

 2) copy it to a neutral place and restructure the PAM module to use that place

Hi,

@Martin, infopipe works completely asynchronous, calling a child process and waiting for a reply won't block the main process.

@Allison, p11_child already has the options --verification and --certificate to verify a random certificate given as base64 encoded string on the command line.

As already said, adding this verification to infopipe is not difficult and straight forward. I only need a time frame for planning, basically, do you need this during the next month or is it ok if it happens in the last quarter of this year?

bye,
Sumit

> infopipe works completely asynchronous

Nice! That will make things much easier indeed.

> ok if it happens in the last quarter of this year?

That's sufficient, due to how RHEL releases align. Thanks!

(In reply to Martin Pitt from comment #12)
> > infopipe works completely asynchronous
> 
> Nice! That will make things much easier indeed.
> 
> > ok if it happens in the last quarter of this year?
> 
> That's sufficient, due to how RHEL releases align. Thanks!

Hi Martin,

which RHEL release to you have in mind?

bye,
Sumit

Upstream PR: https://github.com/SSSD/sssd/pull/5852

@mpitt can you take a look at the interface that I've proposed in the aforementioned PR? It would be nice to know if it fills your expectations. By the way, if you want to start working on your side of the solution I can provide you with a sssd build that includes the changes.

@sgadekar can you review the test provided in the second commit of the PR? Link: https://github.com/SSSD/sssd/pull/5852/commits/9699a4bd42bfc2490a30fab9ec179d84e38f1688

Thanks a lot Iker for working on this!

The API is certainly very convenient, I am just not sure about how useful the error message is with a "real" but untrusted certificate. I followed up on https://github.com/SSSD/sssd/pull/5852#pullrequestreview-793030991 , let's discuss the finer details there to keep it in one place?

Pushed PR: https://github.com/SSSD/sssd/pull/5852

* `master`
    * 50e6070e4784e4a9a01ce26bd08229a07557948a - Tests: ifp interface to validate certificate
    * cf75d897b8ef03fdc471059214e86824f19b1bd1 - ifp: new interface to validate a certificate

@ipedrosa I've emailed you with the test-results I'm noticing. Please check it.

I tested this in https://github.com/SSSD/sssd/issues/5224#issuecomment-987917472 ff. This generally works, but there's still two hiccups. The fatal one is that (1) it is impossibly hard to tell from outside if a CA database was configured in sssd (as that doesn't happen by default when joining an IPA domain), and (2) FindByValidCertificate() does not have an usable error code in this case. (2) feels like the much simpler bug, so I filed this as https://github.com/SSSD/sssd/issues/5911 .

(In reply to Martin Pitt from comment #28)
> I tested this in
> https://github.com/SSSD/sssd/issues/5224#issuecomment-987917472 ff. This
> generally works, but there's still two hiccups. The fatal one is that (1) it
> is impossibly hard to tell from outside if a CA database was configured in
> sssd (as that doesn't happen by default when joining an IPA domain), and (2)

Hi,

I just want to point out that Smartcard authentication is not enabled on IPA clients by default, you have to run a dedicated script to enable Smartcard authentication on IPA servers and clients which then will create the CA certficate file as well.

I'm not sure what would be best with respect to Cockpit, always try to offer Smartcard authentication or only offer it if the system has it enabled as well?

bye,
Sumit



> FindByValidCertificate() does not have an usable error code in this case.
> (2) feels like the much simpler bug, so I filed this as
> https://github.com/SSSD/sssd/issues/5911 .

Sumit: One has to explicitly enable cert authentication.  So we'll certainly update the documentation to point out that you need to do that "cp /etc/ipa/ca.crt /etc/sssd/pki/sssd_auth_ca_db.pem" step after the "realm join". That should take care of most use cases, it's just another step which is easy to forget/get wrong and then debug.

However, my big concern (the "fatal") is not actually new installs -- these can be taken care of with documentation. But moving from FindByCertificate() to FindByValidCertificate() (as required to solve this CVE) will also affect existing systems, and I'm quite sure that many/most of them will not have a properly configured sssd_auth_ca_db.pem. So for these, auth would stop working on upgrades, without a clear error message.

I sent https://github.com/cockpit-project/cockpit/pull/16703 which uses this new API. I made it such that I added documentation that admins have to tell sssd about the user certificate with this new version. I'm not sure if we can get away with that, or if it's considered too much of a regression on upgrades (mostly on RHEL) -- but I think I'd like to land this at least upstream and in Fedora, and in RHEL 9. More fine-grained error messages are a possible follow-up for 8.6.

Pushed PR: https://github.com/SSSD/sssd/pull/5918

* `master`
    * fd0f087ada2f4a59d4b22846d06f1b19fd522e11 - ifp: improve FindByValidCertificate() error
    * 8d54b8c029d276801b323dfe045a155a4311ee49 - man: update ifp options for FindByValidCertificate

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (new packages: sssd), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2022:4015