-
Notifications
You must be signed in to change notification settings - Fork 239
[Feature] Add BIP-353 proof validation #798
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
base: dev
Are you sure you want to change the base?
Changes from all commits
464e9e9
fa04fc3
529804c
6520a2d
b752082
6e027f1
05bcebd
4133241
2828156
0b71277
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,7 +133,7 @@ def run(self): | |
# Everything is set. Stop the loading screen | ||
if self.loading_screen: | ||
self.loading_screen.stop() | ||
|
||
# Run the overview screen | ||
selected_menu_num = self.run_screen( | ||
PSBTOverviewScreen, | ||
|
@@ -160,6 +160,7 @@ def run(self): | |
return Destination(PSBTNoChangeWarningView) | ||
|
||
else: | ||
if psbt_parser.dnssec_proof: return Destination(PSBTDnsNameView) | ||
return Destination(PSBTMathView) | ||
|
||
|
||
|
@@ -204,6 +205,38 @@ def run(self): | |
|
||
|
||
|
||
class PSBTDnsNameView(View): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This name may be too generic. It'd make sense to use something more related to BIP-353. Maybe |
||
def run(self): | ||
from seedsigner.gui.screens.screen import LargeIconStatusScreen | ||
from embit.bip353 import verify_dns_proof | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line will raise an error until diybitcoinhardware/embit#102 gets merged |
||
|
||
psbt_parser: PSBTParser = self.controller.psbt_parser | ||
if not psbt_parser: | ||
# Should not be able to get here | ||
return Destination(MainMenuView) | ||
|
||
dnssec_data = psbt_parser.dnssec_proof | ||
dns_name = dnssec_data[0].decode('utf-8').replace('@', '.user._bitcoin-payment.') | ||
proof = dnssec_data[1] | ||
|
||
verified = verify_dns_proof(dns_name+'.', proof) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understood it correctly and as explained in #768 , the dns_name should not be extracted from the proof as it may be different of the dns_name of the PSBT. When validating, we look if the proof allows that name. The way I am doing this is to first look if the verification was successful and then check if the proof allows for that name. I do that in the next line when checking if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see now, you are right. |
||
if not 'error' in verified and len(verified['verified_rrs']) > 0: # Check if there is a valid record for that name in the proof | ||
selected_menu_num = LargeIconStatusScreen( | ||
title = _("DNS name verification"), | ||
button_data = [ButtonOption("Review math")], | ||
text = _("Verification complete. Review the DNS name."), | ||
status_headline = dns_name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will print out SHOULD display |
||
).display() | ||
else: | ||
return Destination(PSBTDnsNameErrorView) | ||
|
||
if selected_menu_num == RET_CODE__BACK_BUTTON: | ||
return Destination(BackStackView) | ||
else: | ||
return Destination(PSBTMathView) | ||
|
||
|
||
|
||
class PSBTMathView(View): | ||
""" | ||
Follows the Overview pictogram. Shows: | ||
|
@@ -594,3 +627,32 @@ def run(self): | |
|
||
if selected_menu_num == RET_CODE__BACK_BUTTON: | ||
return Destination(BackStackView) | ||
|
||
|
||
|
||
class PSBTDnsNameErrorView(View): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same about the naming. As it is an error view, should use a name that identifies the reason of error which is an invalid DNSSEC proof for the provided HRN. Maybe |
||
SELECT_DIFF_SEED = ButtonOption("Return") | ||
|
||
def run(self): | ||
psbt_parser: PSBTParser = self.controller.psbt_parser | ||
if not psbt_parser: | ||
# Should not be able to get here | ||
return Destination(MainMenuView) | ||
|
||
# Just a WarningScreen here; only use DireWarningScreen for true security risks. | ||
selected_menu_num = self.run_screen( | ||
WarningScreen, | ||
title=_("DNS name Error"), | ||
status_icon_name=SeedSignerIconConstants.WARNING, | ||
status_headline=_("DENSSEC verification Failed"), | ||
text=_("Verification not compelted. Verify the DNS name and the proof."), | ||
button_data=[self.SELECT_DIFF_SEED] | ||
) | ||
|
||
if selected_menu_num == 0: | ||
# clear seed selected for psbt signing since it did not add a valid signature | ||
self.controller.psbt_seed = None | ||
return Destination(MainMenuView, clear_history=True) | ||
|
||
if selected_menu_num == RET_CODE__BACK_BUTTON: | ||
return Destination(MainMenuView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will raise an error in all PSBT parser test cases until diybitcoinhardware/embit#102 gets merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should I handle this? Put a try and except?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this functionality depends on the
embit
PR, this PR should only be merged into SeedSigner once those changes are merged intoembit
. So there’s no immediate need to add temporary error handling here.