Skip to content

Forbid SHA-1 digests as part of RFC 9904 changes - #3069

Merged
gbrodman merged 1 commit into
google:masterfrom
gbrodman:9904
Jun 1, 2026
Merged

gbrodman merged 1 commit into
google:masterfrom
gbrodman:9904

Conversation

@gbrodman

@gbrodman gbrodman commented May 28, 2026 •

Copy link
Copy Markdown
Collaborator

We can't change digest types that are already in the database but that's fine (since we just store them as integers). But we forbid them as part of domain creates/updates.


This change is Reviewable

@gbrodman
gbrodman force-pushed the 9904 branch 3 times, most recently from 3196c22 to 52a2031 Compare May 29, 2026 17:25
@gbrodman
gbrodman requested a review from CydeWeys May 29, 2026 17:31

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@CydeWeys made 2 comments.
Reviewable status: 0 of 25 files reviewed, 2 unresolved discussions (waiting on gbrodman).


core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 379 at r1 (raw file):

  }

  public static boolean algorithmIsInvalid(int alg) {

Curious as to why this was inverted?


core/src/main/java/google/registry/tools/DigestType.java line 32 at r1 (raw file):

 */
public enum DigestType {
  // Algorithm number 1 is SHA-1 and is deliberately NOT SUPPORTED.

We need to lock this change behind a FeatureFlag and send out notification 30 days ahead of time.

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@gbrodman made 2 comments.
Reviewable status: 0 of 25 files reviewed, 2 unresolved discussions (waiting on CydeWeys).


core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 379 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Curious as to why this was inverted?

Suggested by IDEA, because all calls to it were immediately inverted.


core/src/main/java/google/registry/tools/DigestType.java line 32 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

We need to lock this change behind a FeatureFlag and send out notification 30 days ahead of time.

Done. We'll use the same feature flag for the algorithms too.

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Geez that's a lot of test changes ... too bad we were using a SHA1 digest everywhere.

@CydeWeys made 2 comments and resolved 2 discussions.
Reviewable status: 0 of 28 files reviewed, 1 unresolved discussion (waiting on gbrodman).


core/src/main/java/google/registry/tools/DigestType.java line 45 at r2 (raw file):

  private final int wireValue;
  private final int bytes;

You could consider adding a third field here, something like boolean allowedInRfc, which would then make it easier to see in the constructor calls above what's allowed and what's not, and then the filter at the call site would be easier as it'd just call filter on the getter.

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@CydeWeys reviewed 28 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on gbrodman).

We can't change digest types that are already in the database but that's
fine (since we just store them as integers). But we forbid them as part
of domain creates/updates.

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no kidding :(

@gbrodman made 2 comments.
Reviewable status: 26 of 28 files reviewed, 1 unresolved discussion (waiting on CydeWeys).


core/src/main/java/google/registry/tools/DigestType.java line 45 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

You could consider adding a third field here, something like boolean allowedInRfc, which would then make it easier to see in the constructor calls above what's allowed and what's not, and then the filter at the call site would be easier as it'd just call filter on the getter.

yeah sure, we'll be doing something similar when forbidding insecure algorithms too

@gbrodman
gbrodman added this pull request to the merge queue Jun 1, 2026
Merged via the queue into google:master with commit dde4107 Jun 1, 2026
14 of 15 checks passed
@gbrodman
gbrodman deleted the 9904 branch June 1, 2026 18:45
gbrodman added a commit to gbrodman/nomulus that referenced this pull request Jun 16, 2026
This is similar to PR google#3069 but for the algorithms themselves rather
than the digest data. This forbids algorithms, that, according to RFC
9904, should not be used.
CydeWeys pushed a commit to CydeWeys/nomulus that referenced this pull request Jun 16, 2026
This is similar to PR google#3069 but for the algorithms themselves rather
than the digest data. This forbids algorithms, that, according to RFC
9904, should not be used.
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.

2 participants