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.

Sign up for GitHub

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

Jump to bottom

storage: Reword Tang fingerprint verification dialog #18908

Merged
merged 1 commit into from
Jul 3, 2023
Merged

storage: Reword Tang fingerprint verification dialog #18908

merged 1 commit into from
Jul 3, 2023

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Jun 8, 2023 •
edited
Loading

No description provided.

@mvollmer mvollmer force-pushed the storage-tang-trust-refresh branch from a643e17 to d5bed79 Compare June 8, 2023 15:28
jelly
jelly reviewed Jun 8, 2023
View reviewed changes
Copy link
Member

@jelly jelly left a comment

Choose a reason for hiding this comment

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

I assume we drop SHA1 is some mark it as insecure. Seems the tests still need to be adjusted to match the reworded test.

@mvollmer
Copy link
Member Author

mvollmer commented Jun 9, 2023

I assume we drop SHA1 is some mark it as insecure.

The thinking was that everybody can produce SHA256 fingerprints on the Tang server, and showing the SHA1 fingerprint doesn't have any value anymore, and just complicates the dialog.

@mvollmer mvollmer force-pushed the storage-tang-trust-refresh branch from d5bed79 to 0703d7d Compare June 9, 2023 09:30
@mvollmer
Copy link
Member Author

mvollmer commented Jun 9, 2023

The thinking was that everybody can produce SHA256 fingerprints on the Tang server,

Yeah, except debian-stable, rhel-8, and centos-8, hmm...

@mvollmer mvollmer marked this pull request as draft June 9, 2023 11:41
@mvollmer mvollmer force-pushed the storage-tang-trust-refresh branch from 0703d7d to 5914abe Compare June 13, 2023 10:10
@mvollmer mvollmer changed the title storage: Drop SHA1 from Tang fingerprint verification dialog storage: Reword Tang fingerprint verification dialog Jun 13, 2023
@mvollmer mvollmer force-pushed the storage-tang-trust-refresh branch from 5914abe to d760b61 Compare June 13, 2023 10:20
@mvollmer mvollmer mentioned this pull request Jun 15, 2023
Merged
@mvollmer
Copy link
Member Author

I have left SHA-1 in the dialog. We can't say which algorithm will be used by tang-show-keys, and some supported OSes still use SHA-1.

@mvollmer
Copy link
Member Author

Looks like this now:

image

@mvollmer mvollmer marked this pull request as ready for review June 19, 2023 13:40
@mvollmer mvollmer requested a review from garrett June 20, 2023 12:06
@mjahoda
Copy link

mjahoda commented Jun 22, 2023

I would change the wording a bit:

  1. Check the key hash with the Tang server. -> Compare the key hash with the key hash of the Tang server.
  2. In a terminal, run: -> In a terminal, enter:

@mvollmer
Copy link
Member Author

I would change the wording a bit:

1. Check the key hash with the Tang server. -> Compare the key hash with the key hash of the Tang server.

2. In a terminal, run: -> In a terminal, enter:

@garrett, what do you think? To me, it's all the same. :-)

@mvollmer mvollmer force-pushed the storage-tang-trust-refresh branch from d760b61 to c543772 Compare June 29, 2023 12:03
cockpituous
cockpituous reviewed Jun 29, 2023
View reviewed changes
pkg/storaged/crypto-keyslots.jsx
Comment on lines +626 to +627
<TextContent>
<Text component={TextVariants.p}>{_("Check the key hash with the Tang server.")}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 added lines are not executed by any test. Details

pkg/storaged/crypto-keyslots.jsx
Comment on lines +629 to +631
<Text component={TextVariants.h3}>{_("How to check")}</Text>
<Text component={TextVariants.p}>
{_("In a terminal, run: ")}
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 added lines are not executed by any test. Details

pkg/storaged/crypto-keyslots.jsx
Comment on lines +638 to +641
</Text>
<Text component={TextVariants.p}>
{_("Check that the SHA-256 or SHA-1 hash from the command matches this dialog.")}
</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

These 4 added lines are not executed by any test. Details

pkg/storaged/crypto-keyslots.jsx
Comment on lines +643 to +644
<Text component={TextVariants.h3}>{_("SHA-256")}</Text>
{ sigkey_thps.map(s => <Text key={s.sha256} component={TextVariants.pre}>{s.sha256}</Text>) }
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 added lines are not executed by any test. Details

pkg/storaged/crypto-keyslots.jsx
Comment on lines +646 to +648
<Text component={TextVariants.h3}>{_("SHA-1")}</Text>
{ sigkey_thps.map(s => <Text key={s.sha1} component={TextVariants.pre}>{s.sha1}</Text>) }
</TextContent>
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 added lines are not executed by any test. Details

@mvollmer mvollmer merged commit 678b9fd into cockpit-project:main Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cockpituous cockpituous cockpituous left review comments

@jelly jelly jelly approved these changes

@garrett garrett Awaiting requested review from garrett

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mvollmer @mjahoda @jelly @cockpituous