Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Submit challenge with poet cert pubkey hint #6313

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

ConvallariaMaj
Copy link
Contributor

Motivation

Add cetifier pubkey hint to poet Submit request to support changes on poet service side spacemeshos/poet#501

Description

Pass first 4 bytes of cerifier pubkey as hint for poet + new tests.

Test Plan

  • unittests

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed
  • Update changelog as needed

activation/poet.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
activation/poet_client_test.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.8%. Comparing base (516fc47) to head (8302f4c).

Files with missing lines Patch % Lines
activation/poet.go 75.0% 2 Missing and 1 partial ⚠️
activation/certifier.go 0.0% 1 Missing ⚠️
sql/localsql/certifier/db.go 66.6% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #6313     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.1%     
=========================================
  Files          343     343             
  Lines        44505   44514      +9     
=========================================
- Hits         35548   35546      -2     
- Misses        6952    6970     +18     
+ Partials      2005    1998      -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@poszu poszu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in poet to support extra trusted keys was backward compatible - the poet will accept registrations with certs signed by the main certifier without the hint. Hence, I think this change is not really required.

It would be more interesting to allow loading certificates from disk and using them instead of certifying at the certifier service (then the hint would be required).

Comment on lines +503 to +508
info, err := c.getInfo(ctx)
if err != nil {
return nil, err
}

return &PoetAuth{PoetCert: cert, CertPubKey: info.Certifier.Pubkey}, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should continue to work the way it was (without passing the hint).

Copy link
Member

@fasmat fasmat Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand? Sure the node just chose to not provide the certifier pubkey hint and then everything works like before (after the fix in this PR: spacemeshos/poet#523), but if the nodes certificate has been signed by a different Certifier than the one returned from /info of the PoET it is using then it should be able to at least try to still submit in case the PoET accepts certificates from the certifier the node is using or not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean: if the cert is signed by the certifier exposed on info.Certfier.URL, then you don't need to pass the hint (legacy support). If it is not signed by this certifier, then passing info.Certifier.Pubkey makes no sense (as it will be rejected)

@fasmat fasmat requested a review from jellonek as a code owner October 9, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants