[Feature] Secondary fallback for package signature verification #3453
Conversation
michalpristas
added
enhancement
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
There was a problem hiding this comment.
Aside from one specific comment in the code, why don't we have an abstraction over the available PGP keys we can use for validating ?
Something like a PGPKeyProvider that would return all the available PGP keys (the builtin one, the one retrieved from internet, the one from fleet, the one from command line...) and return the available ones so that the verifier can try them in order to validate the software?
Wouldn't that make this also easier to test ?
| if strings.HasPrefix(host, string(ProtocolHTTPS)+"://") || | ||
| strings.HasPrefix(host, string(ProtocolHTTP)+"://") { | ||
| return host + "/" + c.config.Path | ||
| } |
There was a problem hiding this comment.
Nit: wouldn't at this point be better to use a url.Parse to extract the various parts and then url.JoinPath the configured path ?
There was a problem hiding this comment.
i see where you're heading. don't have benchmarks but for this simple case it feels like parse and join (which does another fancy things internally) like overkill.
There was a problem hiding this comment.
I'm wondering why have you added this extra case. Is it a valid case the protocol on host to be optional?
Normally I'd agree with Paolo, but as this function does not return an error and url.Parse returns one, it'd not be possible to handle the error here. Therefore, I like how you did it.
changelog/fragments/1695289867-Secondary-fallback-for-package-signature-verification.yaml
Outdated
Show resolved
Hide resolved
| if strings.HasPrefix(host, string(ProtocolHTTPS)+"://") || | ||
| strings.HasPrefix(host, string(ProtocolHTTP)+"://") { | ||
| return host + "/" + c.config.Path | ||
| } |
There was a problem hiding this comment.
I'm wondering why have you added this extra case. Is it a valid case the protocol on host to be optional?
Normally I'd agree with Paolo, but as this function does not return an error and url.Parse returns one, it'd not be possible to handle the error here. Therefore, I like how you did it.
…signature-verification.yaml Co-authored-by: Anderson Queiroz <[email protected]>
Co-authored-by: Anderson Queiroz <[email protected]>
Co-authored-by: Anderson Queiroz <[email protected]>
|
SonarQube Quality Gate
|
Adds a capability to check for fleet server hosted PGP as the last resort in case elastic artifact API is unavailable. Fleet server will host this PGP per version. any snapshot flags or other version qualifiers are ignored. (cherry picked from commit cdca211)
… (#3497) Adds a capability to check for fleet server hosted PGP as the last resort in case elastic artifact API is unavailable. Fleet server will host this PGP per version. any snapshot flags or other version qualifiers are ignored. (cherry picked from commit cdca211) Co-authored-by: Michal Pristas <[email protected]>
…on (elastic#3453) (elastic#3497)" This reverts commit 635b147.
…on (elastic#3453)" This reverts commit cdca211.
…rification (elastic#3453)" (elastic#3509)" This reverts commit e8dca50.
What does this PR do?
This PR adds a capability to check for fleet server hosted PGP as the last resort in case elastic artifact API is unavailable.
Fleet server will host this PGP per version. any snapshot flags or other version qualifiers are ignored.
Why is it important?
Add ability to upgrade securiely in Air gapped environment where fleet server is only reachable URI.
Checklist
./changelog/fragmentsusing the changelog toolFixes: #3264