Skip to content

Add CertificateAuthentication #9756

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 54 commits into from
Jun 1, 2019
Merged

Add CertificateAuthentication #9756

merged 54 commits into from
Jun 1, 2019

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Apr 25, 2019

Initial port of the code with all unit tests passing and some stuff commented out so we can track diffs in git.

Take a look at the commented out changes i needed to make for tests @blowdart

Next steps will be to remove all the commented out code and move the headerforwarding into the existing middleware

@blowdart
Copy link
Contributor

Couple of things to be deleted, but yea, fair enough

@HaoK
Copy link
Member Author

HaoK commented Apr 25, 2019

Ok I'll delete all the commented out stuff next so its easy for you to see what got removed in the next commit

@Eilon Eilon added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Apr 25, 2019
@Eilon
Copy link
Contributor

Eilon commented Apr 25, 2019

When this is non-draft, please tag @aspnet/build to review the package addition and other infra changes.

@HaoK HaoK force-pushed the security-cert branch from 36925f2 to 7ebf3fd Compare May 17, 2019 21:29
@HaoK HaoK marked this pull request as ready for review May 17, 2019 21:41
@HaoK HaoK requested review from analogrelay and a team as code owners May 17, 2019 21:41
@HaoK HaoK mentioned this pull request May 17, 2019
Closed
Tratcher
Tratcher requested changes May 18, 2019
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

PM code 😢

Also get rid of the MessagePack submodule change.

src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs

protected override Task HandleChallengeAsync(AuthenticationProperties properties)
{
// Certificate authentication takes place at the connection level. We can't prompt once we're in
Copy link
Member

Choose a reason for hiding this comment

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

Let's put an event here for the Identity auth selection case. For HttpSys a dev could call AuthenticateAsync (in the selection case it would not have been called) and it would trigger a browser prompt on the same request. If that failed then a 403 is warranted. If it succeeded they could redirect to the next stage of sign-in and the cert would be preserved in the server for the next request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we file a separate issue/ask to add support for this later? This doesn't sound like something that needs to be in the initial chcekpoint, @blowdart thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

But only http.sys. While that'd be nice, we can't make kestrel do it, so yes, nice to have only

src/Security/Authentication/Certificate/src/README.md
@@ -0,0 +1,239 @@
# Microsoft.AspNetCore.Authentication.Certificate
Copy link
Member

Choose a reason for hiding this comment

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

Move to a doc PR/bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah docs with this will come later, readme seems ok to leave in the meantime

@HaoK HaoK mentioned this pull request May 22, 2019
Closed
@HaoK HaoK force-pushed the security-cert branch from 43033f5 to efa5ee6 Compare May 22, 2019 20:00
@HaoK HaoK changed the title [WIP] Initial snapshot of CertAuth (tests pass) Add CertificateAuthentication May 30, 2019
@HaoK
Copy link
Member Author

HaoK commented May 30, 2019

@blowdart @Tratcher thinks the behavior for the certificate forwarder should pickup the header and use that as an override for any existing cert, which is the opposite of the behavior you had before (which would fallback to the header). For now I went with the override behavior, but you guys can discuss what's the ideal behavior

@Tratcher Tratcher mentioned this pull request May 30, 2019
Closed
Tratcher
Tratcher approved these changes May 30, 2019
View reviewed changes
src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs Outdated
Items =
{
{
CertificateAuthenticationDefaults.CertificateItemsKey, certificate.GetRawCertDataString()
Copy link
Member

Choose a reason for hiding this comment

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

Removed?

@HaoK HaoK mentioned this pull request May 31, 2019
Open
@HaoK
Copy link
Member Author

HaoK commented May 31, 2019

@aspnet/build any ideas why I'm getting this error on ci builds only for this PR?

/.azure/pipelines/helix-test.yml: Could not find /.azure/pipelines/jobs/default-build.yml in repository self hosted on https://api.github.com using commit 5ff3b2aeadbc1775f718f2938171f96f5a9b3f84. GitHub reported the error, "Not Found"

@dougbu
Copy link
Contributor

dougbu commented May 31, 2019

any ideas why I'm getting this error on ci builds only for this PR?

It's not only your PR and it's not only this repo e.g. https://github.com/aspnet/EntityFrameworkCore/pull/15878/checks?check_run_id=139437341. It appears to be GitHub flakiness but I'm not sure.

@natemcmaster
Copy link
Contributor

@HaoK just sent mail about this problem, which is affecting everyone, not just this PR. AzDO + GitHub are investigating.

@natemcmaster
Copy link
Contributor

/azp run AspNetCore-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dcarr42 dcarr42 mentioned this pull request May 31, 2019
Closed
@HaoK
Copy link
Member Author

HaoK commented Jun 1, 2019

/azp run AspNetCore-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HaoK HaoK merged commit b75b892 into master Jun 1, 2019
@ghost ghost deleted the security-cert branch June 1, 2019 05:49
This was referenced Jun 1, 2019
Closed
Closed
@JamesNK JamesNK mentioned this pull request Jun 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@Eilon Eilon Eilon left review comments

@blowdart blowdart blowdart left review comments

@natemcmaster natemcmaster natemcmaster left review comments

@bartonjs bartonjs bartonjs left review comments

@dcarr42 dcarr42 dcarr42 left review comments

@Tratcher Tratcher Tratcher approved these changes

@analogrelay analogrelay Awaiting requested review from analogrelay

Assignees
No one assigned
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@HaoK @blowdart @Eilon @dougbu @natemcmaster @davidfowl @Tratcher @bartonjs @dcarr42