Reorder groups in cert Subjects #18837

simo5 · 2018-03-05T16:32:10Z

This is to workaround Bug #18715, which is caused by Golang Crypto's x509
certificate generation ordering Subjects RDN incorrectly and GNUTLS'
bug that "fixes" client certs on read with the correct encoding.

To avoid issues until both are fixed we set the correct ordering ourself

Fixes #18715
xref golang/go#24254 https://gitlab.com/gnutls/gnutls/issues/403#note_61687722

simo5 · 2018-03-05T16:32:35Z

@martinpitt PTAL

mrogers950 · 2018-03-05T16:37:58Z

/lgtm

enj · 2018-03-05T16:40:31Z

If there is a bug in Go itself, should not we fix this generically in:

origin/pkg/cmd/server/crypto/crypto.go

Line 595 in 3addbb0

Organization: u.GetGroups(),

by forcing the ordering that DER expects? Otherwise any other cert we generate with more than one organization will have this issue.

simo5 · 2018-03-05T16:41:50Z

We might do that as well, let's see if the bug goes away with this simple fix first.
Ideally we get golang fixed and we do not need no complex workaround code.

enj · 2018-03-05T16:43:56Z

Ideally we get golang fixed and we do not need no complex workaround code.

If the Go bug gets fixed today, it will take 3 - 6+ months to show up in origin.

martinpitt · 2018-03-05T16:54:00Z

Thanks, this looks like an okay first bandaid!

This still feels a bit unpredictable, though - is there any chance this could not be a set (using the + syntax) but a plain good old list with two O: entries? Same as what would be generated by -subj '/O=system:masters/O=system:cluster-admins/CN=system:admin'?

enj · 2018-03-05T16:56:07Z

is there any chance this could not be a set

The DER encoding is handled by the Go standard library, which uses the SET notation and is not configurable.

simo5 · 2018-03-05T16:59:40Z

@martinpitt I see no reason why we are trying to use multivalued RDNs, @liggitt may know if there is a good reason. We could certainly just have many RDNs and a deeper tree, but this PR fixes the iimediate Pain in an easy backportable way.
Once we know how we want to proceed we can make more complex changes in a different PR?

liggitt · 2018-03-05T17:21:11Z

pkg/cmd/server/admin/default_certs.go

@@ -145,7 +145,7 @@ func DefaultClusterAdminClientCertInfo(certDir string) ClientCertInfo {
 		},
 		UnqualifiedUser: "admin",
 		User:            "system:admin",
-		Groups:          sets.NewString(bootstrappolicy.ClusterAdminGroup, bootstrappolicy.MastersGroup),
+		Groups:          sets.NewString(bootstrappolicy.MastersGroup, bootstrappolicy.ClusterAdminGroup),


A string set is an order independent map. You cannot depend on order in a go map. This could easily change in different go versions or in different invocations (map iteration order is randomized per invocation)

It can never be simple :-)

liggitt · 2018-03-05T17:21:31Z

/hold

simo5 · 2018-03-05T17:32:09Z

@liggitt would it be ok to change ClientCertInfo to use []string instead of sets.String ?
Or is it preferable, at this point to introduce and explit reordering function in userToSubject() ?

liggitt · 2018-03-05T17:49:19Z

Golang Crypto's x509 certificate generation ordering Subjects RDN incorrectly

can you clarify the bug here? either order should result in a valid x509 cert, right?

liggitt · 2018-03-05T17:53:44Z

at this point to introduce and explit reordering function in userToSubject() ?

userToSubject isn't involved... the code that turns ClientCertInfo into a cert template for creation is at
https://github.com/openshift/origin/blob/master/pkg/cmd/server/admin/create_mastercerts.go#L306 and calls clientCertInfo.Groups.List(), which returns the groups in sorted order.

enj · 2018-03-05T18:06:51Z

userToSubject isn't involved

It is the last thing we do to set the cert subject, but the order fixing could be done elsewhere. For example:

origin/pkg/cmd/server/crypto/crypto.go

Line 740 in 95e2620

Subject: subject,

could have the fixup logic.

simo5 · 2018-03-05T18:09:09Z

@liggitt you need to go deeper, that code ends up calling https://github.com/openshift/origin/blob/master/pkg/cmd/server/crypto/crypto.go#L555 which ends up calling userToSubject() in signCertificate()

simo5 · 2018-03-05T18:12:22Z

can you clarify the bug here? either order should result in a valid x509 cert, right?

If golang crypto correctly implemented DER encoding for certs then we wouldn't have to do anything special on our side. Unfortunately golang's pkix.Name defines RelativeDistinguishedNameSET as a simple SET instead of a SET OF and then does *not* order it according to DER rules. DER requires SET OF to be ordered (by length of string in this case, shortest strings first).

liggitt · 2018-03-05T18:18:50Z

that code ends up calling https://github.com/openshift/origin/blob/master/pkg/cmd/server/crypto/crypto.go#L555 which ends up calling userToSubject() in signCertificate()

userToSubject just passes through whatever order was set in CreateClientCertOptions, it doesn't impose any ordering at all

simo5 · 2018-03-05T18:22:00Z

@LiGgit I know userToSubject() does not impose any ordering, but it is the place where all caller of signCertificate ends up into. If I change only CreateClientCertOptions I may miss some code paths.

liggitt · 2018-03-05T18:26:07Z

@LiGgit I know userToSubject() does not impose any ordering, but it is the place where all caller of signCertificate ends up into. If I change only CreateClientCertOptions I may miss some code paths.

I see. Something like:

func userToSubject(u user.Info) pkix.Name {
	// Sort shortest first to workaround go DER encoding bug
	groups := u.GetGroups()
	sort.Slice(groups, func(i, j int) bool {
		if l := len(groups[i]) - len(groups[j]); l != 0 {
			return l < 0
		}
		return groups[i] < groups[j]
	})

	return pkix.Name{
		CommonName:   u.GetName(),
		SerialNumber: u.GetUID(),
		Organization: groups,
	}
}

liggitt · 2018-03-05T18:27:12Z

I don't object to that. if you do that, please reference the go issue and reproducer so we don't lose track of it

simo5 · 2018-03-05T18:28:37Z

@liggitt Yeah that's what I was planning with a big FAT comment on why it is needed

mrogers950 · 2018-03-05T18:56:18Z

golang/go#24254

simo5 · 2018-03-05T19:16:54Z

@LiGgit PTAL and lift hold if you are ok with this

simo5 · 2018-03-05T19:17:25Z

@liggitt [damn fingers stop messing with is name :-)] PTAL

simo5 · 2018-03-05T21:01:56Z

/retest

simo5 · 2018-03-05T22:10:47Z

/retest

simo5 · 2018-03-06T14:32:34Z

/retest

simo5 · 2018-03-06T14:33:49Z

Flake #16832

liggitt · 2018-03-06T15:51:59Z

pkg/cmd/server/crypto/crypto.go

+	// between the encoding of two group RDNs will end up being the encoded
+	// length which is directly related to the group name's length. So we'll
+	// sort such that shortest names come first.
+	groups := u.GetGroups()


actually, make a new slice and copy the groups into it, so we don't share memory and introduce the possibility of races.

This is to workaround Bug openshift#18715, which is caused by Golang Crypto's x509 certificate generation ordering Subjects RDN incorrectly *and* GNUTLS' bug that "fixes" client certs on read with the correct encoding. To avoid issues until both are fixed we set the correct ordering on our own... Signed-off-by: Simo Sorce <[email protected]>

liggitt · 2018-03-06T16:43:18Z

/hold cancel
/lgtm

openshift-ci-robot · 2018-03-06T16:43:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mrogers950, simo5

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cmd/OWNERS~~ [liggitt]

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

simo5 · 2018-03-06T17:34:33Z

/retest

simo5 · 2018-03-06T20:01:57Z

/retest

simo5 · 2018-03-06T21:46:01Z

/retest

openshift-merge-robot · 2018-03-07T23:08:29Z

Automatic merge from submit-queue (batch tested with PRs 18835, 18857, 18641, 18656, 18837).

openshift-ci-robot requested review from 0xmichalis and smarterclayton March 5, 2018 16:32

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 5, 2018

openshift-ci-robot assigned mrogers950 Mar 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2018

liggitt reviewed Mar 5, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2018

simo5 force-pushed the RDNOrder branch from 2d5d27e to 4b702bd Compare March 5, 2018 18:51

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 5, 2018

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2018

simo5 changed the title ~~Reorder groups in Admin cert~~ Reorder groups in cert Subjects Mar 5, 2018

simo5 force-pushed the RDNOrder branch from 4b702bd to 6f483ed Compare March 5, 2018 19:46

liggitt reviewed Mar 6, 2018

View reviewed changes

simo5 force-pushed the RDNOrder branch from 6f483ed to f3e52f9 Compare March 6, 2018 16:16

openshift-ci-robot assigned liggitt Mar 6, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 6, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2018

openshift-merge-robot merged commit 746e4d3 into openshift:master Mar 7, 2018

martinpitt mentioned this pull request Mar 28, 2018

SSL authentication fails against OpenShift 3.9 cockpit-project/cockpit#8638

Closed

martinpitt mentioned this pull request Apr 6, 2018

3.9 alpha 3/4 regression: Certificate verification fails with GnuTLS #18715

Closed

Reorder groups in cert Subjects #18837

Reorder groups in cert Subjects #18837

Conversation

simo5 commented Mar 5, 2018 • edited by enj Loading

simo5 commented Mar 5, 2018

mrogers950 commented Mar 5, 2018

enj commented Mar 5, 2018

simo5 commented Mar 5, 2018

enj commented Mar 5, 2018

martinpitt commented Mar 5, 2018

enj commented Mar 5, 2018

simo5 commented Mar 5, 2018

liggitt Mar 5, 2018

Choose a reason for hiding this comment

simo5 Mar 5, 2018

Choose a reason for hiding this comment

liggitt commented Mar 5, 2018

simo5 commented Mar 5, 2018 • edited Loading

liggitt commented Mar 5, 2018

liggitt commented Mar 5, 2018

enj commented Mar 5, 2018

simo5 commented Mar 5, 2018

simo5 commented Mar 5, 2018 via email

liggitt commented Mar 5, 2018

simo5 commented Mar 5, 2018

liggitt commented Mar 5, 2018

liggitt commented Mar 5, 2018

simo5 commented Mar 5, 2018

mrogers950 commented Mar 5, 2018

simo5 commented Mar 5, 2018

simo5 commented Mar 5, 2018

simo5 commented Mar 5, 2018

simo5 commented Mar 5, 2018

simo5 commented Mar 6, 2018

simo5 commented Mar 6, 2018

liggitt Mar 6, 2018

Choose a reason for hiding this comment

simo5 Mar 6, 2018

Choose a reason for hiding this comment

liggitt commented Mar 6, 2018

openshift-ci-robot commented Mar 6, 2018

simo5 commented Mar 6, 2018

simo5 commented Mar 6, 2018

simo5 commented Mar 6, 2018

openshift-merge-robot commented Mar 7, 2018

simo5 commented Mar 5, 2018 •

edited by enj

Loading

simo5 commented Mar 5, 2018 •

edited

Loading