Cleanup fixed time and allow using time offset for crypto operations

[KaQuMiQ]

Rename "latestServerTime" to "fixedTime" which better describes actual behavior (it was already setting fixed time for all time related operations). Allow using custom time offset for all time related operations in order to compensate client and server time difference.

[KaQuMiQ]

I have also cleaned up time mocking in tests. It was a bit inconsistent and error prone.

[KaQuMiQ]

Ok, I have also cleaned up internal usage of time. It is now much simpler and I believe it was not used properly in one place: https://github.com/ProtonMail/gopenpgp/pull/239/files#diff-272d3efde9c97e3f529b9386d56cd495e9d376b40fb514548fe48396570dc766R96 password.go line 96 was using default configuration time instead of internal one

[twiss]

Hey :wave: Thanks for the PR and the work on this! However, this would be a breaking change to the API, as well as changing the intended use case of the functions.

Just to give some background on why the functions (and naming) are this way, and how we use them - though I don't mean to imply you should use gopenpgp that way - we continually poll the server time, and pass that to UpdateTime. The offset for key generation is specifically to make sure that generated keys are valid for all senders, even if they have a clock that's a bit behind. So it's not meant to compensate for client and server time differences, UpdateTime is meant for that.

But - of course I understand that this setup might not work for you, and it's a bit overly specific, perhaps. So we could try to make the time handling more generic.

We are anyway planning to overhaul and simplify the API, which has been growing organically for a while, in a new major version (v3). The first ideas for that can be found in this branch.

Perhaps, rather than having global variables and functions for setting the time (offset), we could simply add a time paramater to each of the handles / params proposed there, allowing to more easily pass a time for each individual operation. And then, for your use case you could simply pass time.Now().Add(time.Second * offset). Would that work for you?

(And, in the meantime while that API isn't ready yet, you could call UpdateTime() with that value before each operation - which of course I understand is a bit annoying but at least it can be done today without any breaking API changes :blush:)

[KaQuMiQ]

Thank you for explanation @twiss 😄! This is a bit odd then, since this function used to behave "correctly" using server time before while not making time a constant (it was few years ago, probably initial version looking at the file history), but now it no longer updates the server time to compensate the difference but instead sets fixed time for the whole framework. I know that this PR makes a breaking change but this is intentional to prevent incorrect function use and force update. Anyway, if you have to keep those functions (for proper semver without a major version you probably have to) I can put them back with the same behavior, next to the new ones but this seems to be a bit strange to do so when knowing that this is not working as expected. I will at least leave a note there 😇 . The Issue I have and try to solve here is that the client time is sometimes ahead of server time while I have to support multiple servers at once. Proper time management in the current implementation is not possible since you can only set the time which is in the future comparing to previously set time. UpdateTime function besides making time a constant (without possibility bo bring it back to normal) does not allow making change back in time which is an issue I have to fix somehow. I am a bit surprised that you had no similar issues yet (client and server being out of sync ahead or behind a few seconds seems not to be that rare). However, what I really need is to synchronize server and client time to a degree where time won't make any major issues. Manually updating time to a new constant before each operation seems to be an overkill when I know the difference and don't wan't to or can't update it all the time before each operation. How stable is the v3? To be honest current time management done here is like asking for a troubles 😅 if the new v3 allows passing time in function arguments instead that would be much better and solve most of the related issues I have encoutered.

[KaQuMiQ]

I have put back previous public functions to prevent breaking changes and added additional unit tests to ensure previous behavior.

[twiss]

This is a bit odd then, since this function used to behave "correctly" using server time before while not making time a constant (it was few years ago, probably initial version looking at the file history), but now it no longer updates the server time to compensate the difference but instead sets fixed time for the whole framework.

Yes, we used to "progress" the time relatively to the last call to UpdateTime, but this caused issues on certain platforms, sometimes leading to the time being far in the future. The time being in the future is far more problematic than it being a bit behind, because generated keys and signatures will be invalid (for a while). We call UpdateTime regularly, so the clock will never be far behind. If you're worried about it, you can call it before every operation / call to gopenpgp.

The Issue I have and try to solve here is that the client time is sometimes ahead of server time while I have to support multiple servers at once.

Right, but UpdateTime is not really meant to be called with the client time. If the client time is ahead, you probably shouldn't use it at all. If you have a reliable monotonic clock on your platforms, you could use it to add a delta to the latest server time you received, but this normally shouldn't cause the resulting time to drift ahead of the server time.

The reason why we enforce the time to be monotonically increasing is that if, for example, you first generate a key and then sign a message using it, and the signature creation time is behind the key creation time, the signature won't verify.

How stable is the v3?

Not at all, it's still a work in progress.

To be honest current time management done here is like asking for a troubles

This time management is in fact the most reliable method we've found to make sure all cryptographic artefacts are usable (albeit at a small cost to the precision of the timestamps, which I think is not a huge deal).

if the new v3 allows passing time in function arguments instead that would be much better and solve most of the related issues I have encoutered.

OK :+1: Let's go with that, then. In the meantime, do you think you could live with calling UpdateTime before each operation?

---

I do agree it's a bit weird that you can't revert to client time currently, so maybe we could add an escape hatch to UpdateTime to allow passing 0, to do so?

And then, if you really need non-monotonically increasing time for some reason, you could call UpdateTime(0); UpdateTime(newTime). While it's a bit ugly, that way we don't need to add redundant functions for this. What do you think?

[KaQuMiQ]

I should have explained my issue a bit better 😉 The issue I have and try to solve here is time difference between the client and the server which causes i.e. signatures to be invalid. However, the client time being both ahead and behind will cause those. If I sign a message with the client with time ahead of the server time, server refuses to accept this because signature is not yet valid. If the client is behind, it can fail verifying signatures received from server as those may yet be invalid from client perspective. This can also apply for the opposite case so the client being behind can send valid signatures to the server that are no longer valid from the server perspective but that usually requires greater time difference between sides.

From what you have explained to me it looks like you are solving this issue by forcing the client time to be equal to whatever server time you have received from the last response / synchronization which is fine and solves the issue (regardless of the client time being ahead or behind, it will eventually be in sync) as long as you update it all the time. But this is fine only if you synchronize time with one server or multiple servers that are for sure in time sync. When you have more than one server and those have time out of sync between them, communicating with one can force you to update the time to the future while communicating with the second one will force you to update time to the past from the previous server perspective. This is impossible with the current implementation of UpdateTime, not to mention that supporting multiple servers at once is not possible at all because of the time being stored as a single global variable.

Right, but UpdateTime is not really meant to be called with the client time. If the client time is ahead, you probably shouldn't use it at all. If you have a reliable monotonic clock on your platforms, you could use it to add a delta to the latest server time you received, but this normally shouldn't cause the resulting time to drift ahead of the server time.

I am trying not to call UpdateTime at all since once you start using it you are forced to rely on it all the time and that is not a thing I would want to do. I don't get the server time frequently or periodically, so I can synchronize only once in a while or it would force me to do unnecessary requests just to synchronize it. I could instead call UpdateTime with client time and offset which I received when communicating with the server but it still does not allow me to go back in time or reset the time which is required when you have multiple servers that can be out of sync (even among themselves). I am aware of that the time changes might negatively impact the validity of crypto operations but the thing is that I have to adjust to more than one server and those adjustments might require different time to be used (both in future and in the past). I could also wait on the client side a few seconds to compensate for the difference in some cases, but this is not a good solution (and terrible for client UX to force people to wait a few extra seconds for no actual reason).

This time management is in fact the most reliable method we've found to make sure all cryptographic artefacts are usable (albeit at a small cost to the precision of the timestamps, which I think is not a huge deal).

It seems that you are already handling time on the client side but the framework uses a global variable instead of allowing client to specify the time when executing operations. Global mutable variables are almost always bad 😉 in this case you have to manage it, use dedicated locking and can't use more than one time source at the same time (which shouldn't be the case but unfortunately I have exactly that issue).

I do agree it's a bit weird that you can't revert to client time currently, so maybe we could add an escape hatch to UpdateTime to allow passing 0, to do so?

And then, if you really need non-monotonically increasing time for some reason, you could call UpdateTime(0); UpdateTime(newTime). While it's a bit ugly, that way we don't need to add redundant functions for this. What do you think?

I can remove SetFixedTime function if you don't like it. I don't need it that much, this was added as a replacement for UpdateTime to be 100% clear what it actually does 😇 What I actually need is the ability to add a time diff to all operations while not using constant time. This is the exact thing that SetTimeOffset function which I have added does. Is that fine to keep SetTimeOffset function as it adds a new functionality without impacting existing ones?

[KaQuMiQ]

I have made setFixedTime internal - it is easier to use in tests than manually assigning time in pgp struct. However it won't be available as a new function in the public interface.