From d6c48bf62bbcc027907729b673a31ca279fc9d49 Mon Sep 17 00:00:00 2001 From: Koushik Narayanan Date: Sat, 8 Aug 2020 22:44:20 +0530 Subject: [PATCH] - Added a ClockSkew time.Duration field to SAMLServiceProvider - Factor clockskew in Assertion validation (Conditions and SubjectConfirmationData) - Added a function to manipulate time values in test data for testing --- saml.go | 1 + saml_test.go | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++ validate.go | 10 ++++-- 3 files changed, 102 insertions(+), 3 deletions(-) diff --git a/saml.go b/saml.go index 516fb14..1a20c8c 100644 --- a/saml.go +++ b/saml.go @@ -66,6 +66,7 @@ type SAMLServiceProvider struct { Clock *dsig.Clock signingContextMu sync.RWMutex signingContext *dsig.SigningContext + ClockSkew time.Duration } // RequestedAuthnContext controls which authentication mechanisms are requested of diff --git a/saml_test.go b/saml_test.go index 0a4d575..5e328f5 100644 --- a/saml_test.go +++ b/saml_test.go @@ -27,6 +27,7 @@ import ( "io/ioutil" "log" "testing" + "time" "github.com/beevik/etree" "github.com/russellhaering/gosaml2/types" @@ -89,6 +90,66 @@ func TestDecode(t *testing.T) { require.EqualValues(t, expected, assertion, "decrypted assertion did not match expectation") } +func signResponseWithTime(t *testing.T, resp string, + sp *SAMLServiceProvider, notBefore time.Time, validity time.Duration) string { + doc := etree.NewDocument() + err := doc.ReadFromBytes([]byte(resp)) + require.NoError(t, err) + + el := doc.Root() + + // Strip existing signatures + signatures := el.FindElements("//Signature") + for _, sig := range signatures { + parent := sig.Parent() + parent.RemoveChild(sig) + } + + // Find the Assertion Element + // and load up the NotBefore, NotOnOrAfter elements + assertionEl := el.SelectElement("saml2:Assertion") + require.NotEmpty(t, assertionEl) + + conditionEl := assertionEl.SelectElement("saml2:Conditions") + require.NotEmpty(t, conditionEl) + + notBeforeEl := conditionEl.SelectAttr("NotBefore") + require.NotEmpty(t, notBeforeEl) + + notAfterEl := conditionEl.SelectAttr("NotOnOrAfter") + require.NotEmpty(t, notAfterEl) + + // Set the time + + notBeforeEl.Value = notBefore.Format(time.RFC3339) + notAfterEl.Value = notBefore.Add(validity).Format(time.RFC3339) + + // SubjectConfirmation elements + + subjectConfirmationEl := assertionEl.FindElement("./saml2:Subject/saml2:SubjectConfirmation/saml2:SubjectConfirmationData") + require.NotEmpty(t, subjectConfirmationEl) + + subjectNotAfterEl := subjectConfirmationEl.SelectAttr("NotOnOrAfter") + require.NotEmpty(t, subjectNotAfterEl) + subjectNotAfterEl.Value = time.Now().Add(validity).Format(time.RFC3339) + + // Sign the modified response + el, err = sp.SigningContext().SignEnveloped(el) + require.NoError(t, err) + + doc0 := etree.NewDocument() + doc0.SetRoot(el) + doc0.WriteSettings = etree.WriteSettings{ + CanonicalAttrVal: true, + CanonicalEndTags: true, + CanonicalText: true, + } + + str, err := doc0.WriteToString() + require.NoError(t, err) + return str +} + func signResponse(t *testing.T, resp string, sp *SAMLServiceProvider) string { doc := etree.NewDocument() err := doc.ReadFromBytes([]byte(resp)) @@ -276,6 +337,39 @@ func TestSAML(t *testing.T) { _, err = sp.ValidateEncodedResponse(base64.StdEncoding.EncodeToString([]byte(missingIDResponse))) require.Error(t, err) require.Equal(t, "Missing ID attribute", err.Error()) + + // ClockSkew test + // Setup a SAML response with NotBefore in the future + assertionInFuture := signResponseWithTime(t, rawResponse, sp, time.Now().Add(3*time.Minute), 1*time.Hour) + + assertionInFutureInfo, err := sp.RetrieveAssertionInfo(base64.StdEncoding.EncodeToString([]byte(assertionInFuture))) + require.NoError(t, err) + // Should give error as NotBefore is in the future + require.True(t, assertionInFutureInfo.WarningInfo.InvalidTime) + + //Adjust clock skew and test for expiration + sp.ClockSkew = 4 * time.Minute + assertionInFutureInfo, err = sp.RetrieveAssertionInfo(base64.StdEncoding.EncodeToString([]byte(assertionInFuture))) + require.NoError(t, err) + require.False(t, assertionInFutureInfo.WarningInfo.InvalidTime) + + // Reset Clock skew + sp.ClockSkew = 0 + + // Setup an Assertion that has NotBefore 1 hour in the past, with NotOnOrAfter set to 55 minutes + expiredAssertion := signResponseWithTime(t, rawResponse, sp, time.Now().Add(-1*time.Hour), 55*time.Minute) + expiredAssertionInfo, err := sp.RetrieveAssertionInfo(base64.StdEncoding.EncodeToString([]byte(expiredAssertion))) + require.NoError(t, err) + // Should give error as Assertion expired 5 minutes back + require.True(t, expiredAssertionInfo.WarningInfo.InvalidTime) + + // Setup a clockskew of 6 minutes and test for expiration + sp.ClockSkew = 6 * time.Minute + expiredAssertionInfo, err = sp.RetrieveAssertionInfo(base64.StdEncoding.EncodeToString([]byte(expiredAssertion))) + require.NoError(t, err) + require.False(t, expiredAssertionInfo.WarningInfo.InvalidTime) + + sp.ClockSkew = 0 } func TestInvalidResponseBadBase64(t *testing.T) { diff --git a/validate.go b/validate.go index 537d0ce..661bfce 100644 --- a/validate.go +++ b/validate.go @@ -76,7 +76,9 @@ func (sp *SAMLServiceProvider) VerifyAssertionConditions(assertion *types.Assert return nil, ErrParsing{Tag: NotBeforeAttr, Value: conditions.NotBefore, Type: "time.RFC3339"} } - if now.Before(notBefore) { + // If now is lesser than notBefore, then see if the difference + // is greater than allowed clockskew + if now.Sub(notBefore) < 0 && notBefore.Sub(now) > sp.ClockSkew { warningInfo.InvalidTime = true } @@ -89,7 +91,9 @@ func (sp *SAMLServiceProvider) VerifyAssertionConditions(assertion *types.Assert return nil, ErrParsing{Tag: NotOnOrAfterAttr, Value: conditions.NotOnOrAfter, Type: "time.RFC3339"} } - if now.After(notOnOrAfter) { + // if now - notOnOrAfter is greater than allowed clock skew + // note: if ClockSkew is not set, then it will be 0. + if now.Sub(notOnOrAfter) > sp.ClockSkew { warningInfo.InvalidTime = true } @@ -229,7 +233,7 @@ func (sp *SAMLServiceProvider) Validate(response *types.Response) error { } now := sp.Clock.Now() - if now.After(notOnOrAfter) { + if now.Sub(notOnOrAfter) > sp.ClockSkew { return ErrInvalidValue{ Reason: ReasonExpired, Key: NotOnOrAfterAttr,