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

Increase MAX_REQ_LEN to allow transport of more sophisticated credent… #611

Closed

Conversation

mdoggydog
Copy link

…ials

The constant MAX_REQ_LEN, which controls the maximum size of a password
accepted by saslauthd, is increased from 256 to 4096. Though a simple
human-typed plaintext password is unlikely to ever be so big, this
allows saslauthd to accept other, larger credentials. In particular,
this is large enough to allow saslauthd to receive SAML2 security
assertions and pass them through to a PAM authenticator for verification.

MAX_REQ_LEN also affects the fixed-size buffers allocated for "login",
"service", "realm", and "client_addr", though only "password" needs
to be enlarged for this use-case. However, at this point it doesn't
seem to be worth the effort/complexity to introduce a separate constant
to independently control the size of the "password" buffer.

@flowerysong
Copy link
Contributor

The constant MAX_REQ_LEN, which controls the maximum size of a password
accepted by saslauthd, is increased from 256 to 4096. Though a simple
human-typed plaintext password is unlikely to ever be so big, this
allows saslauthd to accept other, larger credentials. In particular,
this is large enough to allow saslauthd to receive SAML2 security
assertions and pass them through to a PAM authenticator for verification.

This feels like a use case that could very easily break with this arbitrary limit; I happen to have a random SAML assertion lying around from some debugging I was doing recently and it's far more than 4096 characters:

ironbull-zeke ~ $ wc -c saml
20289 saml
ironbull-zeke ~ $ base64 --decode saml  | wc -c
15215

@mdoggydog
Copy link
Author

Yeah... 4096 was pretty much derived from "2-3x as a big as anything I had encountered in my particular use case". I admit I don't have a good sense of how large things like SAML assertions can get out in the wild.

@quanah
Copy link
Contributor

quanah commented May 5, 2021

Please update your commit(s) to be signed off on in accordance with our DCO. Thanks!

…ials

The constant MAX_REQ_LEN, which controls the maximum size of a password
accepted by saslauthd, is increased from 256 to 65536.  Though a simple
human-typed plaintext password is unlikely to ever be so big, this
allows saslauthd to accept other, larger credentials.  In particular,
this is large enough to allow saslauthd to receive SAML2 security
assertions and pass them through to a PAM authenticator for verification.

MAX_REQ_LEN also affects the fixed-size buffers allocated for "login",
"service", "realm", and "client_addr", though only "password" needs
to be enlarged for this use-case.  However, at this point it doesn't
seem to be worth the effort/complexity to introduce a separate constant
to independently control the size of the "password" buffer.

Signed-off-by: Matt Marjanović <maddog@mir.com>
@mdoggydog mdoggydog force-pushed the maddog/increase-MAX_REQ_LEN branch from 445fa1b to 89d0527 Compare May 10, 2021 15:13
@mdoggydog
Copy link
Author

Please update your commit(s) to be signed off on in accordance with our DCO. Thanks!

Done!

(I also went and upped the size of the MAX_REQ_LEN increase, in response to the first comment.)

@quanah
Copy link
Contributor

quanah commented May 10, 2021

tyvm!

@quanah
Copy link
Contributor

quanah commented Oct 5, 2021

saslauthd is only meant for use with simple passwords, so increasing the size here is incorrect. Ticket based mechanisms should not be using saslauthd at all.

@quanah quanah closed this Oct 5, 2021
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

Successfully merging this pull request may close these issues.

None yet

3 participants