Skip to content

Verify that marksdb verify URL starts with ry.marksdb.org#3115

Open
gbrodman wants to merge 1 commit into
google:masterfrom
gbrodman:nordnUrl
Open

Verify that marksdb verify URL starts with ry.marksdb.org#3115
gbrodman wants to merge 1 commit into
google:masterfrom
gbrodman:nordnUrl

Conversation

@gbrodman

@gbrodman gbrodman commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

This change is Reviewable

Comment thread core/src/test/java/google/registry/tmch/NordnVerifyActionTest.java Fixed
Comment thread core/src/test/java/google/registry/tmch/NordnVerifyActionTest.java Fixed
Comment thread core/src/test/java/google/registry/tmch/NordnVerifyActionTest.java Fixed
Comment thread core/src/test/java/google/registry/tmch/NordnVerifyActionTest.java Fixed
Comment thread core/src/test/java/google/registry/tmch/NordnVerifyActionTest.java Fixed
Comment thread core/src/test/java/google/registry/webdriver/DockerWebDriverExtension.java Dismissed
@gbrodman gbrodman requested a review from CydeWeys June 29, 2026 19:42

@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.

It sounds like the hostname shouldn't be configurable at all then? Just make the configuration option be the path?

@CydeWeys made 1 comment.
Reviewable status: 0 of 8 files reviewed, all discussions resolved.

@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.

Kinda yeah, but that'd require chopping apart the URL that we're provided by marksdb in the NordnUploadTask just to put it back together again here.

@gbrodman made 1 comment.
Reviewable status: 0 of 8 files reviewed, all discussions resolved.

@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 1 comment.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on gbrodman).


core/src/main/java/google/registry/tmch/NordnVerifyAction.java line 112 at r2 (raw file):

  LordnLog verify() throws IOException, GeneralSecurityException {
    String host = Ascii.toLowerCase(url.getHost());
    checkArgument(host.startsWith(MARKSDB_URL_BEGINNING), "Bad URL: %s", url);

Expand on the error message here, and/or add comments?

Under what scenario would the host not start with what it's supposed to?

@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 1 comment.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on CydeWeys).


core/src/main/java/google/registry/tmch/NordnVerifyAction.java line 112 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Expand on the error message here, and/or add comments?

Under what scenario would the host not start with what it's supposed to?

if someone manages to do an attack that makes it through the other layers of security, basically. It's not likely.

@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 1 comment.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on gbrodman).


core/src/main/java/google/registry/tmch/NordnVerifyAction.java line 112 at r2 (raw file):

Previously, gbrodman wrote…

if someone manages to do an attack that makes it through the other layers of security, basically. It's not likely.

What kind of attack? What's the threat model exactly?

@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 1 comment.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on CydeWeys).


core/src/main/java/google/registry/tmch/NordnVerifyAction.java line 112 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What kind of attack? What's the threat model exactly?

If someone manages to spoof our auth successfully they could spoof a URL to which we'd send our marksdb credentials

@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.

:lgtm:

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


core/src/main/java/google/registry/tmch/NordnVerifyAction.java line 112 at r2 (raw file):

Previously, gbrodman wrote…

If someone manages to spoof our auth successfully they could spoof a URL to which we'd send our marksdb credentials

Or presumably if they partially compromised Marksdb's servers, they could use this to then compromise clients' credentials as well?

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.

3 participants