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

Improve URLToPath #408

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Improve URLToPath #408

merged 1 commit into from
Feb 12, 2025

Conversation

codysoyland
Copy link
Member

Summary

Improvements to URLToPath func for wider compatibility.

  • handle http:// scheme
  • cut out colons
  • make filename lowercase
  • added tests

Fixes #407

Release Note

Documentation

- handle http:// scheme
- cut out colons
- make filename lowercase

Signed-off-by: Cody Soyland <[email protected]>
@codysoyland codysoyland requested a review from a team as a code owner February 12, 2025 15:49
fn = fn[8:]
}
fn, _ = strings.CutPrefix(fn, "https://")
fn, _ = strings.CutPrefix(fn, "http://")
Copy link
Contributor

@haydentherapper haydentherapper Feb 12, 2025

Choose a reason for hiding this comment

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

Does the TUF client require a TLS connection? Is that why stripping off the scheme is OK?

I ask because accessing TUF metadata over HTTP shouldn't be an issue as the metadata is signed (same reason why CA certs/CRLs are served over HTTP sometimes).

The root of trust must be the bundled or provided TUF root, not WebPKI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think http:// is used in unit tests (see #407).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this needs more changes, that we can't just drop the scheme and need to plumb through whether or not the connection requires TLS to the TUF client.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just used for the cache directory name, but I can see how we might want to put the scheme in the directory name. The full URL is passed to go-tuf so I think non-TLS is supported there.

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s a niche case to support, maybe this is fine and we just document it? I wouldn’t really expect access to a TUF repo over both http and https and needing multiple distinct caches.

Copy link
Member

Choose a reason for hiding this comment

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

TUF itself does not require TLS, the integrity control is managed via the TUF metadata. Only reason for TLS would be to add confidentiality.

fn = fn[8:]
}
fn, _ = strings.CutPrefix(fn, "https://")
fn, _ = strings.CutPrefix(fn, "http://")
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s a niche case to support, maybe this is fine and we just document it? I wouldn’t really expect access to a TUF repo over both http and https and needing multiple distinct caches.

@codysoyland codysoyland merged commit 0e5547e into main Feb 12, 2025
12 checks passed
@codysoyland codysoyland deleted the url-to-path branch February 12, 2025 20:53
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.

URLToPath is not Windows-safe
3 participants