gh-64551: Add RFC 5034 AUTH support to poplib #142613
Conversation
fatelei
requested review from
a team,
1st1,
AA-Turner,
FFY00,
Fidget-Spinner,
ZeroIntensity,
ambv,
asvetlov,
brandtbucher,
brettcannon,
diegorusso,
ericsnowcurrently,
gpshead,
kumaraditya303,
lysnikolaou,
markshannon,
methane,
ncoghlan,
pablogsal,
picnixz,
savannahostrowski,
tomasr8,
warsaw and
willingc
as code owners
|
Can you please rebase/merge your branch on top of main please? |
diegorusso
requested review from
bitdancer and
warsaw
and removed request for
1st1,
AA-Turner,
FFY00,
Fidget-Spinner,
ZeroIntensity,
asvetlov,
brandtbucher,
diegorusso,
ericsnowcurrently,
kumaraditya303,
lysnikolaou,
markshannon,
ncoghlan,
pablogsal,
picnixz,
savannahostrowski,
tomasr8,
warsaw and
willingc
Thanks! I removed all the reviewers and added only the ones in the original issue. |
bitdancer
left a comment
bitdancer
left a comment
There was a problem hiding this comment.
Thanks for working on this! I have several suggestions, mostly about mimicing the smtplib API for consistency.
| """ | ||
| return self._shortcmd('PASS %s' % pswd) | ||
|
|
||
| def auth(self, mechanism, authobject=None, initial_response=None): |
There was a problem hiding this comment.
If we want to respect the existing module structure this code should go in the "optional commands" section. Since they aren't currently alphabetized I'd put it either before or after the stls command.
| Result is 'response'. | ||
| """ | ||
| if authobject is not None and initial_response is not None: | ||
| raise ValueError('authobject and initial_response are mutually exclusive') |
There was a problem hiding this comment.
I think we should model this on how smtplib works for consistency's sake: authobject when called with None can return an initial response or None. For smtplib we have an initial_response_ok parameter to use if you want to force challenge/response in, say, a testing scenario. Again for consistency it would be nice to copy that API.
When thinking only about implementing the auth command, this may seem a bit strange. But if you look at the auth command as support for a higher level 'login' command (that doesn't exist yet), then it makes more sense (take a look at how smtplib login works). I'd like to see a login command added as well; we don't necessarily need to do it in this PR, but it might make sense.
What we do need here is at least one method that returns an authobject, presumably auth_plain since that is required by the RFC. Again, see smtplib for a model.
| if initial_response is not None: | ||
| if isinstance(initial_response, str): | ||
| initial_response = initial_response.encode(self.encoding) | ||
| b64 = base64.b64encode(initial_response).decode('ascii') |
There was a problem hiding this comment.
Technically per the RFC we should check how long this is going to make the AUTH command, and not do the initial response it if would exceed 255 bytes.
| if isinstance(initial_response, str): | ||
| initial_response = initial_response.encode(self.encoding) | ||
| b64 = base64.b64encode(initial_response).decode('ascii') | ||
| return self._shortcmd(f'AUTH {mechanism} {b64}'.rstrip()) |
There was a problem hiding this comment.
This return doesn't work, since the initial response can be just the start of the challenge/response cycle, not always the end of it.
| return self._shortcmd(f'AUTH {mechanism} {b64}'.rstrip()) | ||
|
|
||
| if authobject is None: | ||
| return self._shortcmd(f'AUTH {mechanism}') |
There was a problem hiding this comment.
For consistency with smtplib (and imaplib) I don't think we should support authobject=None.
| resp = self._getresp() | ||
| if resp[:3] == b'+OK': | ||
| return resp | ||
|
|
There was a problem hiding this comment.
I wonder if we should validate the '+ ' here and raise an error if it isn't that? Most likely the decode would fail if the server response is malformed, but it probably wouldn't hurt to be cautious, from a security perspective.
Lib/poplib.py
Outdated
| challenge = base64.b64decode(challenge_b64) | ||
| except Exception: | ||
| padded = challenge_b64 + b'=' * (-len(challenge_b64) % 4) | ||
| challenge = base64.b64decode(padded, validate=False) |
There was a problem hiding this comment.
We need to check for decode failure here, as well, and if it fails, the RFC requires we send back a '*' response to cancel the auth. And then raise an -ERR, I think.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
4 participants
Add RFC 5034 AUTH support to poplib
📚 Documentation preview 📚: https://cpython-previews--142613.org.readthedocs.build/