New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow certificate claim checking to fall back to using CN #603

Merged
merged 5 commits into from Dec 18, 2015

Conversation

Projects
None yet
6 participants
@iamjasonp
Copy link
Member

iamjasonp commented Dec 10, 2015

We currently check only the Subject Alternative Name when looking at certificates. In order to match RFC 2818, in the event there are no DNS values in the SAN field (or no SAN extension at all), fall back to using CN (this is the behaviour that we use on Desktop pre-4.6)

Relates to #576

Allow certificate claim checking to fall back to using CN
We currently check only the Subject Alternative Name when looking at certificates
In order to match RFC 2818, in the event there are no DNS values in the SAN field
(or no SAN extension at all), fall back to using CN
@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 10, 2015

@dotnet-bot test outerloop please

@roncain

This comment has been minimized.

Copy link
Contributor

roncain commented Dec 10, 2015

LGTM

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 10, 2015

@bartonjs could you also review please?

@bartonjs

This comment has been minimized.

Copy link
Member

bartonjs commented Dec 10, 2015

Algorithmically, you've done the right thing here. The remaining problem is that GetDnsFromExtensions returns things that aren't dNSName values (and doesn't work on Unix). Lacking a SAN extension object to read from semantically (it's on my infinite TODO list), your best bet is to do something like:

  • Pick a SubjectAltNames extension from somewhere, embed the bytes in your library.
    • Let's assume it contains two entries: dNSName:example.com, dNSName:*.example.com
    • Two is the minimum, but make sure that it doesn't contain things that'll trip you up.
  • Make a Lazy
    • Load the bytes into an X509Extension object, get the string representation
    • Find example.com in the output
    • The character before it is the key-value delimiter (= on Windows, : on Unix)
    • The pair-separator sequence is now optional-comma and whatever whitespace (,<space> on Unix (IIRC), \r\n on Windows)
    • The dNSName string value is whatever is after the pair-separator, and before the key-value delimiter.
      • DNS on Unix (not localized)
      • DNS Name on Windows en-US (localized)

Then, in GetDnsFromExtensions, skip over anything that doesn't match the dNSName prefix (for example, the ipAddress entries).

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 11, 2015

@bartonjs yep - was planning to take the rest in steps (since it's logically easier to review, see #576). All of the issues you mentioned there will eventually be dealt with. Given you've pointed this out in this PR though, I'll probably go ahead and implement these changes in one go.

Thanks!

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 11, 2015

@bartonjs - wondering if I could cheat here a bit. Basically we're looking at parsing the following:

<separator(s)><separator(s)>

is it sufficient to figure out that the identifier ("dnsName" or "DNS Name") is the string from the start of string to the first delimiter seen?

I agree it's probably safer to move forward from the end of to the first non-whitespace, not-comma character though, just seeing if assuming we can assume identifier is str[0] .. str[identifier-1] makes the code less ugly :)

Gist here with what I have for the moment: https://gist.github.com/iamjasonp/dbe3966aba99e8bc5d22; I ran this against Linux and it seems to parse out the various elements fine (In the gist, I stripped out a huge bunch of cruft around Console.WriteLine while I was playing around)

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 11, 2015

(gee, brings me back to when I was mucking around with the various XmlReaders/Writers :) )

@bartonjs

This comment has been minimized.

Copy link
Member

bartonjs commented Dec 11, 2015

@iamjasonp Seems legit. I couldn't remember if there was a header on it (or not), but when I just looked at an example there wasn't.

@bartonjs

This comment has been minimized.

Copy link
Member

bartonjs commented Dec 11, 2015

@iamjasonp And, to be pedantic to avoid confusion: dNSName is the identifier in the spec (little d and all); DNS Name is the Windows en-US type identifier, and DNS is the Unix (OpenSSL) type identifier. 😄

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 11, 2015

Pedantic is good 😄
I spent some time reading through the RFC. Whoooeeee... it's a doozy.

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 14, 2015

Update: things are looking good, seems like the OuterLoop test that broke in Linux are working again. I'll update this PR once it's in a state that's presentable, right now it's a big kludge.

iamjasonp added some commits Dec 14, 2015

Claims should only made for dNSNames in the SAN
Previously, we blindly added all items contained in the SubjectAlternativeName,
even though the SAN field could contain entries like iPAddress, eMail, URI, and
we do not want to make claims for entries other than dNSName
Add ability to specify certificate FriendlyName, SubjectAltNames to B…
…ridge

* Allows bridge to specify the FriendlyName of the certificate for identification purposes
* Changes created certs so that endpoints creating certs now also set the FriendlyName
* Allow Subject Alternative Names to be specified separate from the Subject
  for testing purposes - e.g., when we only want to specify the CN and not the SAN
@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 16, 2015

@dotnet-bot test outerloop please

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 16, 2015

I came up with a really clunky way to test this functionality:

  1. Have three certificates:
  • CN = localhost
  • CN = testmachine.example.com
  • CN = testmachine
  • Note that there are no Subject Alt Names for any of these certificates. That's why we needed 2e24d02 to lay a foundation for this commit.
  1. Create endpoints for each of these three, and bind the cert to the endpoint
  2. Run a test through ALL THREE endpoints.
  3. Check that EXACTLY one of these tests will have passed - EXACTLY two will have failed with a MessageSecurityException due to certificate validation issues.

The reason we need to do this is because I haven't found a good way to allow BridgeClient to report to the Bridge what endpoint it believes it's hitting. So the Bridge is listening and doesn't care where the requests are coming in, but there's a difference to the client between hitting "localhost" or "testmachine" or "testmachine.example.com".

I guess in this way, we test the positive and negative cases in one go, so that's a win, right? 😄

public static readonly char Delimiter;
public static readonly string Separator;

// static initializer runs only when one of the properties is accessed

This comment has been minimized.

@iamjasonp

iamjasonp Dec 16, 2015

Author Member

I ran this through the debugger and noted that accessing X509SubjectAlternativeNameConstants.Oid did not call the static initializer. It was called only when one of the readonly fields was accessed.

This comment has been minimized.

@bartonjs

bartonjs Dec 16, 2015

Member

@iamjasonp Yep, const values are embedded as literals at the callsite, the type isn't involved at runtime.

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 16, 2015

@bartonjs - arggh, semantics:

If a subjectAltName extension of type dNSName is present, that MUST be used as the identity. Otherwise, the (most specific) Common Name field in the Subject field of the certificate MUST be used. Although the use of the Common Name is existing practice, it is deprecated and Certification Authorities are encouraged to use the dNSName instead.

So... the current behaviour is that GetDnsFromExtensions will return an empty string array. If there is a SubjectAlternativeName field that contains zero entries, it behaves the same as if there was no SubjectAlternativeName field at all.

Thoughts on whether this is an important enough distinction?

NINJA EDIT - looks like it's "subjectAltName extension of type dNSName is present", so I think the behaviour as implemented is correct.

@bartonjs

This comment has been minimized.

Copy link
Member

bartonjs commented Dec 16, 2015

@iamjasonp I agree with the ninja edit, that you're doing the right thing in the case of SAN:Yes, DNS-SAN:No.

@hongdai

This comment has been minimized.

Copy link
Contributor

hongdai commented Dec 17, 2015

Ideally we want functional tests to be deterministic. But I agree we should not block the checkin as the test does provide desired coverage and it will take some time to investigate a deterministic way. Please open an issue for the test improvement.

From: Jason Pang [mailto:notifications@github.com]
Sent: Wednesday, December 16, 2015 3:39 AM
To: dotnet/wcf wcf@noreply.github.com
Subject: Re: [wcf] Allow certificate claim checking to fall back to using CN (#603)

I came up with a really clunky way to test this functionality:

  1.  Have three certificates:
    
    • CN = localhost
    • CN = testmachine.example.com
    • CN = testmachine

· Note that there are no Subject Alt Names for each of these

  1. Create endpoints for each of these three, and bind the cert to the endpoint
  2. Run a test through ALL THREE endpoints.
  3. Check that EXACTLY one of these tests will have passed - EXACTLY two will have failed with a MessageSecurityException due to certificate validation issues.

The reason we need to do this is because I haven't found a good way to allow BridgeClient to report to the Bridge what endpoint it believes it's hitting. So the Bridge is listening and doesn't care where the requests are coming in, but there's a difference to the client between hitting "localhost" or "testmachine" or "testmachine.example.com".

I guess in this way, we test the positive and negative cases in one go, so that's a win, right? [😄]


Reply to this email directly or view it on GitHubhttps://github.com//pull/603#issuecomment-165078055.

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 17, 2015

@mconnew, could you do a review? Last time @roncain reviewed it was a much different change from today

@iamjasonp iamjasonp force-pushed the iamjasonp:san-check-cn branch from 4e74422 to ecaa68f Dec 17, 2015

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 17, 2015

Ideally we want functional tests to be deterministic.

One of the unfortunate parts about this particular test is that what is "correct" behaviour is only going to be defined at runtime, depending on what parameter you're passing into msbuild

I have, however, modified the test to be a [Theory] test that is a bit better at compartmentalizing what a correct an incorrect variation is.

@iamjasonp iamjasonp force-pushed the iamjasonp:san-check-cn branch 2 times, most recently from 7fb59ea to a9c251d Dec 17, 2015

@iamjasonp iamjasonp force-pushed the iamjasonp:san-check-cn branch from a9c251d to 878c966 Dec 17, 2015

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 17, 2015

@dotnet-bot retest this please

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 17, 2015

@dotnet-bot test outerloop please

return list;
}
}
}

This comment has been minimized.

@hongdai

hongdai Dec 17, 2015

Contributor

Deterministic in the run time is sufficient. Thanks for being creative to solve the issue, Jason! LGTM from testing perspective.

static X509SubjectAlternativeNameConstants()
{
// Extracted a well-known X509Extension
const string x509ExtensionBase64String = "MCSCFW5vdC1yZWFsLXN1YmplY3QtbmFtZYILZXhhbXBsZS5jb20=";

This comment has been minimized.

@mconnew

mconnew Dec 18, 2015

Member

Why not encode this directly as a byte array? Having it as a base64 encoded string doesn't make it any less of an opaque blob.

This comment has been minimized.

@iamjasonp

iamjasonp Dec 18, 2015

Author Member

I'll change this in the next PR, was just a quick way for me to test this which I never changed back


// static initializer runs only when one of the properties is accessed
static X509SubjectAlternativeNameConstants()
{

This comment has been minimized.

@mconnew

mconnew Dec 18, 2015

Member

Can this entire static constructor be placed inside a try/catch and then either set a flag which causes an exception to be thrown when any of the values are attempted to be retrieved such as the delimiter, or simply fail the process? Any exceptions in a static initializer are silently swallowed and can be the source of difficult to track down bugs.

This comment has been minimized.

@iamjasonp

iamjasonp Dec 18, 2015

Author Member

Good point. In order not to muck up the ordering of the commits here I'll merge this and immediately open up another PR to fix this issue.

@mconnew

This comment has been minimized.

Copy link
Member

mconnew commented Dec 18, 2015

LGTM, the only concern is the possibility of having an exception in the static block. I'm okay with a follow up PR to address that though.

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 18, 2015

Thanks @mconnew, @hongdai

I'm trying to figure out why the outerloop build in CI will fail but the same failure doesn't repro locally for me, so there might be some light churn on the test.

EDIT: got it. Retesting now, see #603 (comment) for explanation

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 18, 2015

@dotnet-bot test outerloop please

Add test for certificate claim check fallback to CN
Verifies that when a Subject Alternative Name is not specified in an endpoint certificate,
we are able to fall back to Subject CN to verify the endpoint

@iamjasonp iamjasonp force-pushed the iamjasonp:san-check-cn branch from 878c966 to 4c76923 Dec 18, 2015

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 18, 2015

@dotnet-bot test outerloop please

@iamjasonp

This comment has been minimized.

Copy link
Member Author

iamjasonp commented Dec 18, 2015

FYI: the reason why outerloop tests were failing in CI is because when running a bridge as "localhost", the check for

            var domainNameEndpointUri = new Uri(Endpoints.Tcp_ClientCredentialType_Certificate_With_CanonicalName_DomainName_Address);
            list.Add(new object[] {
                domainNameEndpointUri,
                domainNameEndpointUri.Host.IndexOf('.') == -1});

meant that the Bridge was always presenting URIs as "localhost - and thus the "should test pass" would still resolve as true, as we were only looking for "." in the hostname. Fixed this, and now it passes.

(I usually run the bridge separately, and use the same Bridge instance across multiple runs, so I never ran into this issue. This issue will surface when running .\build.cmd /p:WithCategories=OuterLoop if the Bridge has not been started before)

iamjasonp added a commit that referenced this pull request Dec 18, 2015

Merge pull request #603 from iamjasonp/san-check-cn
Allow certificate claim checking to fall back to using CN

@iamjasonp iamjasonp merged commit 7604283 into dotnet:master Dec 18, 2015

6 checks passed

Linux Debug Build and Test Build finished. No test results found.
Details
Linux Release Build and Test Build finished. No test results found.
Details
Outerloop Windows Debug Build Build finished. 121 tests run, 0 skipped, 0 failed.
Details
Outerloop Windows Release Build Build finished. 119 tests run, 0 skipped, 0 failed.
Details
Windows_NT Debug Build Build finished. 228 tests run, 2 skipped, 0 failed.
Details
Windows_NT Release Build Build finished. 233 tests run, 2 skipped, 0 failed.
Details

@iamjasonp iamjasonp referenced this pull request Jan 6, 2016

Closed

Address certificate validation issues per #422 #576

5 of 5 tasks complete

@iamjasonp iamjasonp deleted the iamjasonp:san-check-cn branch Feb 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment