spacemeshos / poet

Spacemesh PoET service reference implementation
MIT License
22 stars 13 forks source link

Fix no trusted certificates #523

Closed fasmat closed 1 month ago

fasmat commented 1 month ago

While fixing failing tests in https://github.com/spacemeshos/go-spacemesh/pull/6313 I noticed this issue.

If filepath.Walk in LoadTrustedPublicKeys fails r.trustedCertifierKeys will not be updated. This means that if this happens during startup no certifier keys will be trusted, not even the one defined in the config.

This updates the code in a way such that if loading keys from disk fails at least the certifier key from the config will still be valid to be used for submissions.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.6%. Comparing base (6d0d2d5) to head (1843441). Report is 2 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #523 +/- ## ======================================= Coverage 80.6% 80.6% ======================================= Files 27 27 Lines 1868 1869 +1 ======================================= + Hits 1506 1507 +1 Misses 231 231 Partials 131 131 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

poszu commented 1 month ago

If it happens at startup, then poet will shutdown with an error. If it happens on a GRPC, I think the whole call should be ineffective - the keys should remain as they were before the call. That's why the r.trustedCertifierKeys is updated only if everything went well, at the very end of LoadTrustedPublicKeys().

fasmat commented 1 month ago

If it happens at startup, then poet will shutdown with an error. If it happens on a GRPC, I think the whole call should be ineffective - the keys should remain as they were before the call. That's why the r.trustedCertifierKeys is updated only if everything went well, at the very end of LoadTrustedPublicKeys().

I'm not sure this is true, e2e tests in https://github.com/spacemeshos/go-spacemesh/pull/6313 got stuck when verifying certificates and just kept re-trying to submit challenges to a poet that would always respond with certificate invalid (because it had 0 trusted certificates in the r.trustedKeys map). The fix in the test was the addition of this little helper: https://github.com/spacemeshos/go-spacemesh/blob/poet-issue-501/activation/e2e/poet_test.go#L90-L94 and ensuring that the passed directory path points to a directory that actually exists. The change in this PR just makes sure that if (re-)loading certificates fails certificates created from the certifier defined in the config can still be verified.

I still think this should be fixed, or failing to reload certificates will disable PoET to validate any submission even the ones using the certifier that is set in the config.

poszu commented 1 month ago

If it happens at startup, then poet will shutdown with an error. If it happens on a GRPC, I think the whole call should be ineffective - the keys should remain as they were before the call. That's why the r.trustedCertifierKeys is updated only if everything went well, at the very end of LoadTrustedPublicKeys().

I'm not sure this is true, e2e tests in spacemeshos/go-spacemesh#6313 got stuck when verifying certificates and just kept re-trying to submit challenges to a poet that would always respond with certificate invalid (because it had 0 trusted certificates in the r.trustedKeys map). The fix in the test was the addition of this little helper: spacemeshos/go-spacemesh@poet-issue-501/activation/e2e/poet_test.go#L90-L94 and ensuring that the passed directory path points to a directory that actually exists. The change in this PR just makes sure that if (re-)loading certificates fails certificates created from the certifier defined in the config can still be verified.

Poet should shutdown because an error is bubbled up here: https://github.com/spacemeshos/poet/blob/ae0594a2fa604b71fa9617c13efd5f2a7fe059aa/registration/registration.go#L165-L167

I still think this should be fixed, or failing to reload certificates will disable PoET to validate any submission even the ones using the certifier that is set in the config.

That's not the case. If reloading certificates from a directory goes wrong, the existing list of keys remains untouched. This change actually makes the situation worse by removing all keys but the main one.

fasmat commented 1 month ago

But in the scenario I'm talking about TrustedKeysDirPath is "" so the code you shared is never executed, yet every request still fails to verify.

EDIT: and that means my fix isn't complete

fasmat commented 1 month ago

Superseded by https://github.com/spacemeshos/poet/pull/525