Fix handling of krb5_data structs.

[pseudometric]

The caller (at least, MIT Kerberos) expects us to return our data in an existing buffer referred to by the krb5_data struct we get, rather than update that struct with a new buffer of our own.

I ran into this when using pykrb5 with PKINIT, and needed to supply a passphrase for the client's private key via a Krb5Prompt object. It mysteriously failed to decrypt the key even though the passphrase returned by the prompter was right. Eventually I tracked it down to pkinit_crypto_openssl.c. This:

rdat.data = buf;
rdat.length = size;

is where it sets the krb5_data rdat it will pass to us to refer to its own buffer, and if you follow the code you see that it expects the returned data to be in buf. We were instead returning an updated rdat.data, which it never looks at. I was getting this KRB5_TRACE:

PKINIT OpenSSL error: error:0906A068:PEM routines:PEM_do_header:bad password read

(which is actually itself a minor bug; it should say "bad decryption"). With this fix, the PKINIT succeeds.

Since it is now possible for pykrb5_set_krb5_data() to fail if the caller's buffer is not big enough, I changed its signature and raise an exception in this case; I'm not sure if that's done as you would want. Ideally we should come up with a test to catch this; I assume it wasn't caught because it's mocked in the tests, but I haven't looked closely at that yet. And, a good question is: does Heimdal have the same calling convention for this?

[jborean93]

Is it possible to test this out by creating a prompter that returns a massive amount of data guaranteed to be larger than the buffer. We currently do a very simple test for the prompt in https://github.com/jborean93/pykrb5/blob/main/tests/test_creds.py.

[jborean93]

Probably better to use a ValueError rather than a generic Exception.

[pseudometric]

Is it possible to test this out by creating a prompter that returns a massive amount of data…

Indeed, and we should do that, but when I mentioned testing I actually meant something else: a test that triggers the problem that prompted me to discover this bug. I can supply a PKINIT client/KDC configuration if you don't have it already in your test harness and it would be helpful. I'm actually curious as to why, if there are existing tests that actually use the prompt mechanism "for real" (i.e. actually call MIT/Heimdal for it), they aren't already broken. It would be particularly unpleasant if in fact MIT is not consistent about this: some prompt consumers use krb5_data one way, some the other, and the only reason it's all working is that MIT wrote them all and they happen to line up. That would end up being an MIT bug we'd have to report.

This will need some more investigation, of that and of how Heimdal works, plus tests, before it's ready to go.

[jborean93]

I'm actually curious as to why, if there are existing tests that actually use the prompt mechanism "for real" (i.e. actually call MIT/Heimdal for it), they aren't already broken

So the current tests is legitimately using the prompting mechanism for sending the password of a user. It's actually working today but it could be that the different mechanisms using this prompter expect different behaviour, or maybe the PKINIT stuff is stricter? I would have to look into the code to try and figure out what's happening here but unfortunately that's easier said than done. But still it should be possible to have a mock prompter that tries to set a large amount of data to exercise the exception.

One thing to keep in mind is that this is causing a segmentation fault in another call that's using pykrb5_set_krb5_data. The test_get_init_creds_opt_set_salt test is calling get_init_creds_opt_set_salt which in turn calls pykrb5_set_krb5_data. Maybe we need another generic method, one to set the buffer to what's on the input and another to copy the memory like you need for the prompter.

On the plus side test_get_init_creds_password_prompt which uses the mock prompt is passing in the tests so changing the behaviour to copy the buffer seems to be ok for that particular call.

[pseudometric]

Maybe we need another generic method, one to set the buffer to what's on the input and another to copy the memory like you need for the prompter.

Yup, definitely. I had my head down in fixing my specific bug when I made this preliminary patch, but clearly in general there are consumers that require both incompatible update styles for krb5_data; we'll need to implement both.

I would have to look into the code to try and figure out what's happening here but unfortunately that's easier said than done.

Yeah — I spent 5 hours with my own debug builds of both MIT Kerberos and OpenSSL, gdb and plenty of debugging printf()s before I tracked this down. :)