Fix up SSL client certificates #5213 #69 by guruz · Pull Request #5289 · owncloud/client

guruz · 2016-10-31T14:02:25Z

The re-enables the UI, uses Qt API for importing and
stores the certificate/key in the keychain.

mention-bot · 2016-10-31T14:02:27Z

@guruz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @danimo, @nocteau and @krnowak to be potential reviewers.

guruz · 2016-11-30T12:37:32Z

@ogoffart @ckamm @dragotin Would you judge this as OK? On startup..
The server (nginx at least) sends a HTTP 400 if there is no cert sent (for status.php). Then we'll load the credentials and retry (the connection validator seems to retry if reportResult that) when the cert (and other stuff) is fetched from keychain. Then it works..

Does this loading-from-keychain somehow interfere with SAML/Shibboleth or anything overriding the auth via theme? I guess not since the type of credentials is set in the cfg and not keychain?

I don't know about this. Could we generally require that credentials must be loaded before we attempt to connect?

Yes, maybe it's better if this is generally needed, so we'd always load the credentials at the start from keychain.
@ogoffart @dragotin @danimo Any input?

The re-enables the UI, uses Qt API for importing and stores the certificate/key in the system keychain. People who had set up client certs need to re-setup the account. This is ok since it was an undocumented feature anyway.

ckamm · 2016-12-06T09:21:28Z


-    // DM: TLS Client Cert GUI support disabled for now
+
+#if QT_VERSION < QT_VERSION_CHECK(5, 4, 0)


Must be >= 5.4.0?!

No, for old versions we want to hide this button to not allow the user to configure anything.
This is based on 3dd6bcc

ckamm · 2016-12-06T09:25:49Z

-        QString s = QString::fromStdString(certif.Certificate);
-        QByteArray ba = s.toLocal8Bit();
+        // to re-create the session ticket because we added a key/cert
+        acc->setSslConfiguration(QSslConfiguration());


Does this line do anything? In particular, shouldn't the SslConfiguration returned from the next line be exactly the empty configuration passed in here?!

No, setting a null configuration should make getOrCreateSslConfig return a new configuration filled in with some stuff already. The goal is also to clear any ssl session ticket etc out so we start with a new configuration state

@ckamm Do you agree/understand? Then I can merge?

@guruz Yes, right. Setting a null configuration will cause getOrCreateSslConfig to build a fresh configuration! All good.

ckamm · 2016-12-06T09:36:16Z

-const char certifPathC[] = "certificatePath";
-const char certifPasswdC[] = "certificatePasswd";
+const char clientCertificatePEMC[] = "_clientCertificatePEM";
+const char clientKeyPEMC[] = "_clientKeyPEM";


This means this change will force current users of client certificates to re-setup? (probably okay)

Yes... IMHO that's ok because it was always an undocumented feature

on the other hand, why changing?

Because the config file is not a secure place to store a private key

ah oif course

ckamm · 2016-12-06T09:38:32Z

        Q_EMIT fetched();
    } else {
+        // Read client cert from keychain
+        const QString kck = keychainKey(_account->url().toString(), _user + clientCertificatePEMC);


Is it correct that the PEM and the key are both considered secrets?

Maybe not. Do you think I should store the cert in the config? I thought having it all in keychain is more clean

ckamm

Some questions and one thing that looks wrong.

ogoffart

Looks good to me. Nice to see this finally done :-)

ogoffart · 2016-12-08T16:07:22Z

-const char certifPathC[] = "certificatePath";
-const char certifPasswdC[] = "certificatePasswd";
+const char clientCertificatePEMC[] = "_clientCertificatePEM";
+const char clientKeyPEMC[] = "_clientKeyPEM";


on the other hand, why changing?

ogoffart · 2016-12-08T16:10:56Z

+
+    if (readJob->error() == NoError && readJob->binaryData().length() > 0) {
+        QByteArray clientKeyPEM = readJob->binaryData();
+        // FIXME Unfortunately Qt has a bug and we can't just use QSsl::Opaque to let it


Has the bug been reported? If yes, a reference to the bug number here would be nice so we can remove that code in some year when we see it is fixed in the version we support.

guruz added Security DO NOT MERGE YET labels Oct 31, 2016

guruz added this to the 2.3.0 milestone Oct 31, 2016

guruz force-pushed the fix_client_certs branch from 0b6b215 to 707541a Compare November 30, 2016 12:35

guruz commented Nov 30, 2016

View reviewed changes

Fix up SSL client certificates #5213 #69

81631ee

The re-enables the UI, uses Qt API for importing and stores the certificate/key in the system keychain. People who had set up client certs need to re-setup the account. This is ok since it was an undocumented feature anyway.

guruz force-pushed the fix_client_certs branch from 707541a to 81631ee Compare December 5, 2016 16:15

guruz added ReadyToTest QA, please validate the fix/enhancement and removed DO NOT MERGE YET labels Dec 5, 2016

This was referenced Dec 5, 2016

Replace p12topem.* with QSslCertificate::importPkcs12 #5213

Closed

TLS Client certificate support [$110] #69

Closed

ckamm reviewed Dec 6, 2016

View reviewed changes

ckamm suggested changes Dec 6, 2016

View reviewed changes

guruz changed the title ~~WiP: Fix up SSL client certificates #5213 #69~~ Fix up SSL client certificates #5213 #69 Dec 7, 2016

ogoffart reviewed Dec 8, 2016

View reviewed changes

guruz merged commit c6f4f44 into master Jan 2, 2017

guruz deleted the fix_client_certs branch January 3, 2017 11:03

brdns mentioned this pull request Oct 15, 2024

mTLS Client Certificate support #11933

Closed

2 tasks


		// DM: TLS Client Cert GUI support disabled for now

		#if QT_VERSION < QT_VERSION_CHECK(5, 4, 0)

Conversation

guruz commented Oct 31, 2016

Uh oh!

mention-bot commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckamm Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckamm left a comment

Choose a reason for hiding this comment

Uh oh!

ogoffart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ckamm Dec 6, 2016 •

edited

Loading