Repository navigation
Conversation
|
I'm okay with the feature but I don't think we need to mimic Sequoia's API so closely. I think a smaller patch would be just to extend The multitude of arguments may look ugly on the Rust side but on the Python one it's implemented using keyword arguments and only the ones that are specified need to be written down explicitly. Wdyt @dralley ? |
|
No strong feelings, but I agree that a What other options might exist in the future within this API? If it's just for creation time, that's definitely overkill IMO, but if there's other "caution required" options that we might add then it might be deserve some discussion. |
|
My thinking was that this can lead to somewhat unusable Tsk's so that's why I came up with the hazmat idea, but it is easy to simplify it to not expose the builder and just make it a straight option on generate. Let me modify the code to do that. A caution required thing I was toying around with is adding the option to configure KDF parameters when setting passwords (for example to use minimal hashing when creating keys for tests so tests stay quick); that might actually lead to a security weakness (or keys that are unusable with GnuPG in the case of argon) but again that is opt-in so maybe no need for a hazmat module. Such a warning can also be in the documentation. |
It's definitely a good thing to validate the input to see if it fits the constraints but OTOH I think Sequoia will also do checks so invalid input may already fail. In general having a Tsk template that has fields for configuring various options would be a good idea (likewise for signature generation etc) but for such a small thing let's go with the simplest approach. |
|
Agree. If you want to keep this branch around for future additions like the KDF thing, that would be fine. For |
5e0d070 to
9362b92
Compare
|
I simplified it; no need to keep the hazmat idea alive in a stale branch, and it is easily reconstructed if necessary. |
| )); | ||
| } | ||
| let creation_time = creation_time | ||
| .call_method1("astimezone", (PyTzInfo::utc(creation_time.py())?,))? |
There was a problem hiding this comment.
Is it necessary to go through Python datetime here, or can we create the appropriate datetime using Chrono?
There was a problem hiding this comment.
Yes unfortunately - Chrono really wants a datetime with tz=utc, so with a datetime object like
import datetime as dt
import zoneinfo
cursed_dt = dt.datetime.now(tz=zoneinfo.ZoneInfo("Europe/Amsterdam"))
chrono can't convert it and raises
ValueError: expected datetime.timezone.utc
(I got that with this line on the Rust side:
let creation_time = creation_time.extract::<chrono::DateTime<chrono::Utc>>()?;
and the cursed_dt as input)
There was a problem hiding this comment.
I think the PR looks good in general but yeah this conversion is ugly... I'll take a look if pyo3 has some better timezone conversion...
There was a problem hiding this comment.
What do you think about including your example:
import datetime as dt
import zoneinfo
cursed_dt = dt.datetime.now(tz=zoneinfo.ZoneInfo("Europe/Amsterdam"))in the test suite? I see there are some tz tests but they don't look identical... if we have regression tests then I'm happy to merge this now and refactor innards later (but if there are no tests I could break something accidentally)
Unless you think it's already covered then just say so and we're merging this thing 🚀
|
@jap Needs a rebase |
Add a keyword-only creation_time datetime argument, preserving Sequoia defaults when omitted. Validate timezone awareness and OpenPGP timestamp bounds, normalize offsets to UTC, and document validity relative to the chosen time. Signed-off-by: Jasper Spaans <jasper@startmail.com> Assisted-by: OpenAI GPT-6.1 (pi)
9362b92 to
06d2569
Compare
done! |
This branch has not been deployed
3 participants
This PR does two things:
TskBuilderclass to a cryptography-style hazmat module and exposes it there;with_creation_time(...)method to it that maps to Sequoia'sset_creation_time. This then allows setting the creation time for the certificate.Our use case for this is that we'd like to be able to create expired keys as part of our CI jobs (we currently do that with pgpy but we are trying to get rid of that)