Skip to content
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

fix certificate ctx on windows and macOS #39818

Merged
merged 6 commits into from
Aug 10, 2020
Merged

fix certificate ctx on windows and macOS #39818

merged 6 commits into from
Aug 10, 2020

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 23, 2020

This is follow up on #38364
After some search, it seems like there is really no way how to give certificate chain to Schannel.
So instead, we try to add certificates to intermediate CA store. So when Schannel/CAPI needs them, they can get them without network IO.

I updated existing tests to fail if peer does not send full chain (-1)

Fixes #35844

@wfurt wfurt requested review from bartonjs and a team July 23, 2020 05:29
@wfurt wfurt self-assigned this Jul 23, 2020
@ghost
Copy link

ghost commented Jul 23, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

}

// OS failed to build the chain but we have at least some intermediates.
// We will try to add them to "Intermediate Certification Authorities" store.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really want to add all intermediates? Or just the ones that the OS ends up using in a chain?

e.g. Add all of intermediates to chain.ChainPolicy.ExtraStore and build again; then add anything relevant (preferably using a diffing algorithm so that it doesn't potentially reset custom store property overrides).

Using a second chain object (you could copy the policy object over) would make the diff easy, just walk each position and compare that cert.RawData values SequenceEqual.

Ideally, after each add you'd rerun the offline/no-extra build to see if you can avoid invalidating existing higher-scoped entries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(While a diffing algorithm and a while loop sound potentially expensive, most chains are three items long, so there aren't many iterations)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering to add some more complicated logic. But as you said if most chains are 3 long, it does not matter that much. I assumed that adding intermediate once to store is cheap enough so I decided not to care.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one thing to check:

  • Add an intermediate to the store
  • Open the intermediates store in MMC (e.g. add snap-in, certificates, local computer, then pick Intermediate Certificate Authorities / Certificates in the nav tree)
  • Pick that same certificate, right click, Properties
  • Pick "Enable only the following purposes"
  • Toggle some checkmarks
  • Use the API as you have it
  • Right click on the Certificates entry in the nav bar and refresh
  • Reopen the properties dialog.

If the property resets, we need to do complex stuff here. If it doesn't, we're good. (I think we do "merge properties", which should (generally) be safe, but it's good to test that before we run the risk of altering machine state)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that assumes same CA set between runs, right? (right now we generate unique set for each test run and I had situation where it would build up before adding cleanup to the tests)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It assumes that we ever get something in the intermediates collection that's already in the store. E.g. if what we need is a 4 hop chain:

  • root -> intermed1 -> intermed2 -> end-entity

And intermed1 is already in the store, but intermed2 wasn't; we'll end up calling store.Add(intermed1) which will cause the merge logic to apply. Since it changes system persisted state we need to be careful to ensure it's only additive, not destructive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is what I did:

  • modified the test helper to save/load certificates from pfx and I disabled the cleanup
  • I run the test and I check both CA.
  • I removed root CA, run tests and verified that it triggers the adding logic
  • I removed root again and I modified properties on intermediate and I also added friendly name
  • run test again. Root CA was added again and intermediate preserved with my modifications

so the conclusion is that adding existing certificate is not destructive.

@wfurt wfurt changed the title fix certificate ctx on windows fix certificate ctx on windows and macOS Jul 29, 2020
@wfurt
Copy link
Member Author

wfurt commented Jul 29, 2020

I made suggested changes and also fix macOS. That needed more structural changes as the flow is different.
This is ready for another review round.

@wfurt
Copy link
Member Author

wfurt commented Aug 9, 2020

can you please take another look @bartonjs ?

@wfurt wfurt merged commit eb243f0 into dotnet:master Aug 10, 2020
@wfurt wfurt deleted the winChain branch August 10, 2020 19:02
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSLStream should support taking a pre-validated immutable full certificate chain
4 participants