From f6042ce76a8826e7a0760f0cc555a60bbe4c89cc Mon Sep 17 00:00:00 2001 From: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com> Date: Fri, 21 Jul 2023 17:29:07 -0700 Subject: [PATCH] authorize: add "client-certificate-required" reason Add a new reason "client-certificate-required" that will be returned by the invalid_client_certificate criterion in the case that no client certificate was provided. Determine this using the new 'presented' field populated from the Envoy metadata. --- authorize/check_response.go | 9 ++- authorize/check_response_test.go | 15 ++++- authorize/evaluator/evaluator_test.go | 36 ++++++++++- authorize/evaluator/policy_evaluator_test.go | 25 +++++++- pkg/policy/criteria/criteria_test.go | 15 +++-- .../criteria/invalid_client_certificate.go | 33 ++++++++-- .../invalid_client_certificate_test.go | 60 +++++++++++++++++++ pkg/policy/criteria/reasons.go | 51 ++++++++-------- 8 files changed, 202 insertions(+), 42 deletions(-) create mode 100644 pkg/policy/criteria/invalid_client_certificate_test.go diff --git a/authorize/check_response.go b/authorize/check_response.go index b5bd366a5..ecad76de5 100644 --- a/authorize/check_response.go +++ b/authorize/check_response.go @@ -34,7 +34,7 @@ func (a *Authorize) handleResult( ) (*envoy_service_auth_v3.CheckResponse, error) { // If a client certificate is required, but the client did not provide a // valid certificate, deny right away. Do not redirect to authenticate. - if result.Deny.Reasons.Has(criteria.ReasonInvalidClientCertificate) { + if invalidClientCertReason(result.Deny.Reasons) { return a.handleResultDenied(ctx, in, request, result, result.Deny.Reasons) } @@ -93,7 +93,7 @@ func (a *Authorize) handleResultDenied( case reasons.Has(criteria.ReasonRouteNotFound): denyStatusCode = http.StatusNotFound denyStatusText = httputil.DetailsText(http.StatusNotFound) - case reasons.Has(criteria.ReasonInvalidClientCertificate): + case invalidClientCertReason(reasons): denyStatusCode = httputil.StatusInvalidClientCertificate denyStatusText = httputil.DetailsText(httputil.StatusInvalidClientCertificate) } @@ -101,6 +101,11 @@ func (a *Authorize) handleResultDenied( return a.deniedResponse(ctx, in, denyStatusCode, denyStatusText, nil) } +func invalidClientCertReason(reasons criteria.Reasons) bool { + return reasons.Has(criteria.ReasonClientCertificateRequired) || + reasons.Has(criteria.ReasonInvalidClientCertificate) +} + func (a *Authorize) okResponse(headers http.Header) *envoy_service_auth_v3.CheckResponse { var requestHeaders []*envoy_config_core_v3.HeaderValueOption for k, vs := range headers { diff --git a/authorize/check_response_test.go b/authorize/check_response_test.go index b7e837efe..2a1f264c9 100644 --- a/authorize/check_response_test.go +++ b/authorize/check_response_test.go @@ -89,7 +89,7 @@ func TestAuthorize_handleResult(t *testing.T) { }) }) t.Run("invalid-client-certificate", func(t *testing.T) { - // Even if the user is unauthenticated, if a client certificate was required and no valid + // Even if the user is unauthenticated, if a client certificate was required and an invalid // certificate was provided, access should be denied (no login redirect). res, err := a.handleResult(context.Background(), &envoy_service_auth_v3.CheckRequest{}, @@ -101,6 +101,19 @@ func TestAuthorize_handleResult(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 495, int(res.GetDeniedResponse().GetStatus().GetCode())) }) + t.Run("client-certificate-required", func(t *testing.T) { + // Likewise, if a client certificate was required and no certificate + // was presented, access should be denied (no login redirect). + res, err := a.handleResult(context.Background(), + &envoy_service_auth_v3.CheckRequest{}, + &evaluator.Request{}, + &evaluator.Result{ + Allow: evaluator.NewRuleResult(false, criteria.ReasonUserUnauthenticated), + Deny: evaluator.NewRuleResult(true, criteria.ReasonClientCertificateRequired), + }) + assert.NoError(t, err) + assert.Equal(t, 495, int(res.GetDeniedResponse().GetStatus().GetCode())) + }) } func TestAuthorize_okResponse(t *testing.T) { diff --git a/authorize/evaluator/evaluator_test.go b/authorize/evaluator/evaluator_test.go index 06c3eee7c..e1d3a02fb 100644 --- a/authorize/evaluator/evaluator_test.go +++ b/authorize/evaluator/evaluator_test.go @@ -131,9 +131,19 @@ func TestEvaluator(t *testing.T) { // Clone the existing options and add a default client CA. options := append([]Option(nil), options...) options = append(options, WithClientCA([]byte(testCA))) + t.Run("missing", func(t *testing.T) { + res, err := eval(t, options, nil, &Request{ + Policy: &policies[0], + }) + require.NoError(t, err) + assert.Equal(t, NewRuleResult(true, criteria.ReasonClientCertificateRequired), res.Deny) + }) t.Run("invalid", func(t *testing.T) { res, err := eval(t, options, nil, &Request{ Policy: &policies[0], + HTTP: RequestHTTP{ + ClientCertificate: ClientCertificateInfo{Presented: true}, + }, }) require.NoError(t, err) assert.Equal(t, NewRuleResult(true, criteria.ReasonInvalidClientCertificate), res.Deny) @@ -150,11 +160,35 @@ func TestEvaluator(t *testing.T) { }) }) t.Run("client certificate (per-policy CA)", func(t *testing.T) { - t.Run("invalid", func(t *testing.T) { + t.Run("missing", func(t *testing.T) { res, err := eval(t, options, nil, &Request{ Policy: &policies[10], }) require.NoError(t, err) + assert.Equal(t, NewRuleResult(true, criteria.ReasonClientCertificateRequired), res.Deny) + }) + t.Run("invalid (Envoy)", func(t *testing.T) { + res, err := eval(t, options, nil, &Request{ + Policy: &policies[10], + HTTP: RequestHTTP{ + ClientCertificate: ClientCertificateInfo{Presented: true}, + }, + }) + require.NoError(t, err) + assert.Equal(t, NewRuleResult(true, criteria.ReasonInvalidClientCertificate), res.Deny) + }) + t.Run("invalid (authorize)", func(t *testing.T) { + res, err := eval(t, options, nil, &Request{ + Policy: &policies[10], + HTTP: RequestHTTP{ + ClientCertificate: ClientCertificateInfo{ + Presented: true, + Validated: true, + Leaf: testUnsignedCert, + }, + }, + }) + require.NoError(t, err) assert.Equal(t, NewRuleResult(true, criteria.ReasonInvalidClientCertificate), res.Deny) }) t.Run("valid", func(t *testing.T) { diff --git a/authorize/evaluator/policy_evaluator_test.go b/authorize/evaluator/policy_evaluator_test.go index 74d501bc7..efa1a60ea 100644 --- a/authorize/evaluator/policy_evaluator_test.go +++ b/authorize/evaluator/policy_evaluator_test.go @@ -118,7 +118,7 @@ func TestPolicyEvaluator(t *testing.T) { Traces: []contextutil.PolicyEvaluationTrace{{Allow: true}}, }, output) }) - t.Run("invalid cert", func(t *testing.T) { + t.Run("no cert", func(t *testing.T) { output, err := eval(t, p1, []proto.Message{s1, u1, s2, u2}, @@ -129,6 +129,27 @@ func TestPolicyEvaluator(t *testing.T) { IsValidClientCertificate: false, }) require.NoError(t, err) + assert.Equal(t, &PolicyResponse{ + Allow: NewRuleResult(true, criteria.ReasonEmailOK), + Deny: NewRuleResult(true, criteria.ReasonClientCertificateRequired), + Traces: []contextutil.PolicyEvaluationTrace{{Allow: true, Deny: true}}, + }, output) + }) + t.Run("invalid cert", func(t *testing.T) { + output, err := eval(t, + p1, + []proto.Message{s1, u1, s2, u2}, + &PolicyRequest{ + HTTP: RequestHTTP{ + Method: http.MethodGet, + URL: "https://from.example.com/path", + ClientCertificate: ClientCertificateInfo{Presented: true}, + }, + Session: RequestSession{ID: "s1"}, + + IsValidClientCertificate: false, + }) + require.NoError(t, err) assert.Equal(t, &PolicyResponse{ Allow: NewRuleResult(true, criteria.ReasonEmailOK), Deny: NewRuleResult(true, criteria.ReasonInvalidClientCertificate), @@ -241,7 +262,7 @@ func TestPolicyEvaluator(t *testing.T) { require.NoError(t, err) assert.Equal(t, &PolicyResponse{ Allow: NewRuleResult(false), - Deny: NewRuleResult(true, criteria.ReasonAccept, criteria.ReasonInvalidClientCertificate), + Deny: NewRuleResult(true, criteria.ReasonAccept, criteria.ReasonClientCertificateRequired), Traces: []contextutil.PolicyEvaluationTrace{{Deny: true}, {ID: "p1", Deny: true}}, }, output) }) diff --git a/pkg/policy/criteria/criteria_test.go b/pkg/policy/criteria/criteria_test.go index d8b1c829c..103a94b8e 100644 --- a/pkg/policy/criteria/criteria_test.go +++ b/pkg/policy/criteria/criteria_test.go @@ -29,17 +29,22 @@ var testingNow = time.Date(2021, 5, 11, 13, 43, 0, 0, time.Local) type ( Input struct { - HTTP InputHTTP `json:"http"` - Session InputSession `json:"session"` + HTTP InputHTTP `json:"http"` + Session InputSession `json:"session"` + IsValidClientCertificate bool `json:"is_valid_client_certificate"` } InputHTTP struct { - Method string `json:"method"` - Path string `json:"path"` - Headers map[string][]string `json:"headers"` + Method string `json:"method"` + Path string `json:"path"` + Headers map[string][]string `json:"headers"` + ClientCertificate ClientCertificateInfo `json:"client_certificate"` } InputSession struct { ID string `json:"id"` } + ClientCertificateInfo struct { + Presented bool `json:"presented"` + } ) func generateRegoFromYAML(raw string) (string, error) { diff --git a/pkg/policy/criteria/invalid_client_certificate.go b/pkg/policy/criteria/invalid_client_certificate.go index 0dd24f560..8fada3693 100644 --- a/pkg/policy/criteria/invalid_client_certificate.go +++ b/pkg/policy/criteria/invalid_client_certificate.go @@ -7,9 +7,14 @@ import ( "github.com/pomerium/pomerium/pkg/policy/parser" ) -var invalidClientCertificateBody = ast.Body{ +var validClientCertificateBody = ast.Body{ ast.MustParseExpr(`is_boolean(input.is_valid_client_certificate)`), - ast.MustParseExpr(`not input.is_valid_client_certificate`), + ast.MustParseExpr(`input.is_valid_client_certificate`), +} + +var noClientCertificateBody = ast.Body{ + ast.MustParseExpr(`is_boolean(input.http.client_certificate.presented)`), + ast.MustParseExpr(`not input.http.client_certificate.presented`), } type invalidClientCertificateCriterion struct { @@ -25,10 +30,26 @@ func (invalidClientCertificateCriterion) Name() string { } func (c invalidClientCertificateCriterion) GenerateRule(_ string, _ parser.Value) (*ast.Rule, []*ast.Rule, error) { - rule := NewCriterionRule(c.g, c.Name(), - ReasonInvalidClientCertificate, ReasonValidClientCertificate, - invalidClientCertificateBody) - return rule, nil, nil + r1 := c.g.NewRule(c.Name()) + r1.Head.Value = NewCriterionTerm(false, ReasonValidClientCertificate) + r1.Body = validClientCertificateBody + + r2 := &ast.Rule{ + Head: &ast.Head{ + Value: NewCriterionTerm(true, ReasonClientCertificateRequired), + }, + Body: noClientCertificateBody, + } + r1.Else = r2 + + r3 := &ast.Rule{ + Head: &ast.Head{ + Value: NewCriterionTerm(true, ReasonInvalidClientCertificate), + }, + } + r2.Else = r3 + + return r1, nil, nil } // InvalidClientCertificate returns a Criterion which returns true if the diff --git a/pkg/policy/criteria/invalid_client_certificate_test.go b/pkg/policy/criteria/invalid_client_certificate_test.go new file mode 100644 index 000000000..2c9df2e10 --- /dev/null +++ b/pkg/policy/criteria/invalid_client_certificate_test.go @@ -0,0 +1,60 @@ +package criteria + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInvalidClientCertificate(t *testing.T) { + t.Parallel() + + cases := []struct { + label string + input Input + expected A + }{ + { + "not presented", + Input{}, + A{true, A{ReasonClientCertificateRequired}, M{}}, + }, + { + "invalid", + Input{ + HTTP: InputHTTP{ + ClientCertificate: ClientCertificateInfo{Presented: true}, + }, + }, + A{true, A{ReasonInvalidClientCertificate}, M{}}, + }, + { + "valid", + Input{ + HTTP: InputHTTP{ + ClientCertificate: ClientCertificateInfo{Presented: true}, + }, + IsValidClientCertificate: true, + }, + A{false, A{ReasonValidClientCertificate}, M{}}, + }, + } + + const policy = ` +deny: + or: + - invalid_client_certificate: true` + + for i := range cases { + c := cases[i] + t.Run(c.label, func(t *testing.T) { + t.Parallel() + + res, err := evaluate(t, policy, []dataBrokerRecord{}, c.input) + require.NoError(t, err) + assert.Equal(t, A{false, A{}}, res["allow"]) + assert.Equal(t, c.expected, res["deny"]) + }) + } +} diff --git a/pkg/policy/criteria/reasons.go b/pkg/policy/criteria/reasons.go index 08fac08a8..906ebfb08 100644 --- a/pkg/policy/criteria/reasons.go +++ b/pkg/policy/criteria/reasons.go @@ -7,31 +7,32 @@ type Reason string // Well-known reasons. const ( - ReasonAccept = "accept" - ReasonClaimOK = "claim-ok" - ReasonClaimUnauthorized = "claim-unauthorized" - ReasonCORSRequest = "cors-request" - ReasonDeviceOK = "device-ok" - ReasonDeviceUnauthenticated = "device-unauthenticated" - ReasonDeviceUnauthorized = "device-unauthorized" - ReasonDomainOK = "domain-ok" - ReasonDomainUnauthorized = "domain-unauthorized" - ReasonEmailOK = "email-ok" - ReasonEmailUnauthorized = "email-unauthorized" - ReasonHTTPMethodOK = "http-method-ok" - ReasonHTTPMethodUnauthorized = "http-method-unauthorized" - ReasonHTTPPathOK = "http-path-ok" - ReasonHTTPPathUnauthorized = "http-path-unauthorized" - ReasonInvalidClientCertificate = "invalid-client-certificate" - ReasonNonCORSRequest = "non-cors-request" - ReasonNonPomeriumRoute = "non-pomerium-route" - ReasonPomeriumRoute = "pomerium-route" - ReasonReject = "reject" - ReasonRouteNotFound = "route-not-found" - ReasonUserOK = "user-ok" - ReasonUserUnauthenticated = "user-unauthenticated" // user needs to log in - ReasonUserUnauthorized = "user-unauthorized" // user does not have access - ReasonValidClientCertificate = "valid-client-certificate" + ReasonAccept = "accept" + ReasonClaimOK = "claim-ok" + ReasonClaimUnauthorized = "claim-unauthorized" + ReasonClientCertificateRequired = "client-certificate-required" + ReasonCORSRequest = "cors-request" + ReasonDeviceOK = "device-ok" + ReasonDeviceUnauthenticated = "device-unauthenticated" + ReasonDeviceUnauthorized = "device-unauthorized" + ReasonDomainOK = "domain-ok" + ReasonDomainUnauthorized = "domain-unauthorized" + ReasonEmailOK = "email-ok" + ReasonEmailUnauthorized = "email-unauthorized" + ReasonHTTPMethodOK = "http-method-ok" + ReasonHTTPMethodUnauthorized = "http-method-unauthorized" + ReasonHTTPPathOK = "http-path-ok" + ReasonHTTPPathUnauthorized = "http-path-unauthorized" + ReasonInvalidClientCertificate = "invalid-client-certificate" + ReasonNonCORSRequest = "non-cors-request" + ReasonNonPomeriumRoute = "non-pomerium-route" + ReasonPomeriumRoute = "pomerium-route" + ReasonReject = "reject" + ReasonRouteNotFound = "route-not-found" + ReasonUserOK = "user-ok" + ReasonUserUnauthenticated = "user-unauthenticated" // user needs to log in + ReasonUserUnauthorized = "user-unauthorized" // user does not have access + ReasonValidClientCertificate = "valid-client-certificate" ) // Reasons is a collection of reasons.