verification: add test_verify_tz_aware

[woodruffw]

This adds a TZ awareness test, which is currently failing. I'll use this PR to investigate a fix as well.

[woodruffw]

CC @reaperhulk

[woodruffw]

Thinking about this a bit: the underlying (already identified cause) is this implementation of py_to_datetime:

pub(crate) fn py_to_datetime(
    py: pyo3::Python<'_>,
    val: &pyo3::PyAny,
) -> pyo3::PyResult<asn1::DateTime> {
    Ok(asn1::DateTime::new(
        val.getattr(pyo3::intern!(py, "year"))?.extract()?,
        val.getattr(pyo3::intern!(py, "month"))?.extract()?,
        val.getattr(pyo3::intern!(py, "day"))?.extract()?,
        val.getattr(pyo3::intern!(py, "hour"))?.extract()?,
        val.getattr(pyo3::intern!(py, "minute"))?.extract()?,
        val.getattr(pyo3::intern!(py, "second"))?.extract()?,
    )
    .unwrap())
}

When a datetime object is "naive" this does a reasonable-ish thing (interpreting it as a UTC time). However, when a datetime is "aware" this does the wrong thing (ignoring the specified timezone and instead interpreting the datetime as UTC, resulting in false verifications like this one).

One potential fix here is to check the tzinfo attribute here and, if present, call val.astimezone(timezone.utc) on the datetime object. This will normalize the datetime to its equivalent in UTC for "aware" objects, while preserving the current UTC assumption for "naive" objects.

While more correct, this is technically a behavioral change as well 🙂

[alex]

I think my intuition is that we could just always call astimezone(utc) and then get the attributes

[woodruffw]

I think my intuition is that we could just always call astimezone(utc) and then get the attributes

I can do this, but it'll have a slightly weird effect on existing naive datetimes:

If provided, tz must be an instance of a tzinfo subclass, and its utcoffset() and dst() methods must not return None. If self is naive, it is presumed to represent time in the system timezone.

(source)

In other words, naive datetimes will be treated as local time, rather than the current assumption that they're already UTC. But if that's okay, I can go forward with calling astimezone(utc) unconditionally.

[reaperhulk]

I don't think it would be a good idea to have our existing code start treating naïve datetimes as local. We consider (and have documented) naïve datetimes to be UTC all over our APIs.

@woodruffw is your statement about the behavior change for your proposed approach because it previously would just ignore the TZ and now it will not, which means people who are relying on (undocumented and incorrect) behavior may experience breakage? 😄 If so, I agree it is a behavior change, but a worthwhile one.

[alex]

I think what we want is:

• Naive is UTC
• Any TZ is converted to UTC

[woodruffw]

@woodruffw is your statement about the behavior change for your proposed approach because it previously would just ignore the TZ and now it will not, which means people who are relying on (undocumented and incorrect) behavior may experience breakage? 😄 If so, I agree it is a behavior change, but a worthwhile one.

Yep, exactly that 🙂

I think what we want is:

• Naive is UTC
• Any TZ is converted to UTC

Sounds good -- in that case I think we do need to sniff tzinfo is None first, since the default astimezone behavior will do the wrong thing in that case.

I'll make those changes now!

[alex]

Great.

[reaperhulk]

Since py_to_datetime is used in other parts of the x509 API we may need to expand some test cases in those areas as well to make sure we have the behavior we expect. Possibly doc updates too (e.g. presumably all our X.509 builder APIs will now be tzaware?)

[alex]

base.py has _convert_to_naive_utc_time, which I guess is related.

It might be possible to remove that with this?

[woodruffw]

It might be possible to remove that with this?

Yeah, I'll investigate.

[woodruffw]

I've parameterized the test to add positive and negative cases, and confirmed that they work as expected (and that both fail with the old codepath, as expected).

I'll look into _convert_to_naive_utc_time next.

[woodruffw]

Removing _convert_to_naive_utc_time causes a few tests to fail because of naive/aware comparisons, e.g.:

>       if time < _EARLIEST_UTC_TIME:
E       TypeError: can't compare offset-naive and offset-aware datetimes

These seem moderately annoying to fix, since they happen in Python before we perform our (new) timezone normalization. So maybe it makes sense to leave _convert_to_naive_utc_time for now?

[woodruffw]

(CI failure looks spurious)

[reaperhulk]

Yes, pypy is known flaky right now 😭

[woodruffw]

Sigh, coverage. Fixing.