amdonov / lite-idp

Lightweight SAML Identity Provider
Apache License 2.0
210 stars 48 forks source link

Check that SAML assertions are not expired #25

Closed asprouse5 closed 4 years ago

asprouse5 commented 4 years ago

Add two checks when using artifact binding to verify the SAML assertions are not expired

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 64


Changes Missing Coverage Covered Lines Changed/Added Lines %
idp/artifact.go 6 8 75.0%
<!-- Total: 6 8 75.0% -->
Totals Coverage Status
Change from base Build 60: 0.02%
Covered Lines: 1160
Relevant Lines: 1639

💛 - Coveralls
amdonov commented 4 years ago

I'd have to revisit the specs. Are those conditions required? Looks like this code would blow up if those objects are nil.

asprouse5 commented 4 years ago

I found this document that outlines a spec for Internet X.509 Public Key Infrastructure Certificates: https://tools.ietf.org/html/rfc5280#section-4.1.2.5. There's nothing concrete, but it seems to be that these values must be present.

This article splits properties into sections, the latter being optional, implying that the top section is not optional: https://knowledge.digicert.com/solution/SO4583.html

Furthermore, I believe a time.Time variable (the type for NotBefore and NotOnOrAfter) cannot be nil. Example:

package main

import (
    "fmt"
    "time"
)

func main() {
    var t time.Time
    fmt.Println(t)
    fmt.Println(t.IsZero())
}

// prints:
// 0001-01-01 00:00:00 +0000 UTC
// true

I am open to making any changes as necessary.