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

Client Certificate is requested #7

Closed
reluxa opened this issue Oct 12, 2018 · 8 comments
Closed

Client Certificate is requested #7

reluxa opened this issue Oct 12, 2018 · 8 comments

Comments

@reluxa
Copy link

reluxa commented Oct 12, 2018

When the IDP login page is opened the first time the server asks for client certificates. It would be nice if this behavior could be disabled via the config.yaml.

	tlsConfig := &tls.Config{
		Certificates: []tls.Certificate{cert},
		//Some but not all operations will require a client cert
		ClientAuth: tls.VerifyClientCertIfGiven,
		MinVersion: tls.VersionTLS12,
	}
@amdonov
Copy link
Owner

amdonov commented Oct 13, 2018

I agree. However, my primary use case is artifact binding. In that flow, I authenticate service providers by their certificate rather than verifying XML signatures on the requests. I don't think go allows you to prompt for a certificate on one path but not others, but I could be wrong. I'll revisit this and see if I can up come with a solution. I'm open to suggestions.

@shanesiebken
Copy link

shanesiebken commented Oct 17, 2018

One approach you can take is by manually verifying that there is a certificate on whichever paths require a certificate. i.e. (pulled from a handler I wrote for testing this same problem):

if len(r.TLS.PeerCertificates) == 0 {
	http.Error(w, "No certificate provided with request", 403)
	log.Debugf("Request with no authorization info or certificate failed authentication")
	return
}

It's far from ideal, and I honestly can't say I'd advocate for that to be added in this project, but @reluxa could fork and take a similar approach.

EDIT: This would be paired with the VerifyClientCertIfGiven config as mentioned in the issue. The whole thing could likely be conditionalized with a handler that only does that check wrapped around in cases where it's desired.

@amdonov
Copy link
Owner

amdonov commented Oct 17, 2018

That's what's happening here.

// DefaultArtifactResolveHandler is the default implementation for the artifact resolution handler. It can be used as is, wrapped in other handlers, or replaced completely.

Clients are always prompted for a certificate, but it's the only path that requires them. However, clients don't include certificates in the request if we don't at a minimum request them.

@shanesiebken
Copy link

Oh, with that said - the suggested suggested client auth configuration ought to behave appropriately. VerifyClientCertIfGiven is a bit misleading, in that it does request a certificate (https://golang.org/pkg/crypto/tls/#ClientAuthType), and verifies it if it's given.

@shanesiebken
Copy link

And that said, I saw some odd behavior with that client auth configuration in firefox. I didn't dig around too much in there to understand what was going on, and I can't remember whether or not I verified the behavior on other client auth configs. That's not terribly helpful, but something to potentially be wary of.

@amdonov
Copy link
Owner

amdonov commented Oct 17, 2018

The "suggested" client auth configuration is the current configuration,

ClientAuth: tls.VerifyClientCertIfGiven,

@shanesiebken
Copy link

Whew, I had this issue totally backwards in my head. Thanks for clearing that up and sorry for the confusion.

@amdonov
Copy link
Owner

amdonov commented Oct 17, 2018

I'm going to close this issue. @reluxa, If you don't want artifact binding for your use case, you can change the TLS configuration of the IdP by setting the TLSConfig property on the IdP. I realize it means creating your own binary, but I don't want to allow it via configuration change because of the side effects.

@amdonov amdonov closed this as completed Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants