caddy-security v1.1.17 does not compile

[assistcontrol]

# github.com/crewjam/saml
/root/go/pkg/mod/github.com/crewjam/saml@v0.4.10/schema.go:767:41: not enough arguments in call to etreeutils.TransformExcC14n
        have (*etree.Element, string)
        want (*etree.Element, string, bool)

I know that you've forked crewjam/saml into origin_crewjam_saml, but the schema fix is in a branch that isn't used by caddy-security. As a result, I believe that v1.1.17 can't build.

Given that the bug for the schema error has been sitting for months in @crewjam's repo and caddy-security's build is currently broken, what's your plan? Will caddy-security just not build temporarily until the upstream PR gets merged, or do you intend to merge your schema fix to your fork's main and use that?

[greenpau]

@assistcontrol , try doing go mod tidy followed by go mod verify.

[greenpau]

@assistcontrol , if you are building your own custom binary, add the following to go.mod:

caddy-security/go.mod

Line 154 in 4dd61ac

replace github.com/crewjam/saml v0.4.10 => github.com/greenpau/origin_crewjam_saml v0.4.11-0.20221229165346-936eba92623a

[assistcontrol]

Agreed, that does let caddy-security build on its own. I should have been mentioned originally that I'm doing xcaddy build --with github.com/greenpau/caddy-security. FreeBSD does each build in a clean jail so there's nothing to tidy, and xcaddy doesn't make it easy to alter go.mod as an intermediate step.

[greenpau]

@assistcontrol , try passing the substitution/replace using xcaddy —with.

[assistcontrol]

Wow, I never realized xcaddy could do that. Thanks!

[greenpau]

@assistcontrol , please post your final build command 😃

[assistcontrol]

xcaddy --with github.com/greenpau/caddy-security --with github.com/crewjam/saml@v0.4.10=github.com/greenpau/origin_crewjam_saml@v0.4.11-0.20221229165346-936eba92623a

Works for me, and it's nicely future-proofed because the version is specified: if you update the dep in caddy-security, it won't drag old stuff in.

[greenpau]

working with @crewjam to address this here: crewjam/saml#478

[crewjam]

Just highlighting that as it stands the fix causes a security related test to fail, which is why it has stalled. I haven't looked at the fork but I'd urge a little bit of caution here.

[mattjm]

@crewjam Not having dug into the code too much and not being an expert on XML canonicalization, from what I can see passing "false" to the canonicalization method means comments won't be considered for canonicalization. At first I thought "how could that possibly be a problem?" But it looks like maybe it could be:

https://workos.com/blog/fun-with-saml-sso-vulnerabilities-and-footguns

Depending on the ordering of canonicalization vs signature verification steps. I know SAML pretty well but not your library--if you want to discuss further let me know.

[crewjam]

Thanks @mattjm . I think @greenpau has it figured out.