rfq: auth/transport harmonization pass#2147
Conversation
Bring the portfolio pilot RPC client in line with the price oracle: it
now takes an rfq.TLSConfig built from new
portfoliopilottls{disable,insecure,nosystemcas,certpath} flags and
reuses the existing configureTransportCredentials helper. Previously
the pilot dialed unconditionally with InsecureSkipVerify=true; the new
default is real TLS verification against system roots.
NewRpcPortfolioPilot now accepts an optional macaroon dial option, sourced from a new portfoliopilotmacaroonpath flag. Validate rejects a configured macaroon when pilot TLS is disabled, matching the price oracle.
Replace the duplicated per-service TLS flag sets (priceoracletls* and portfoliopilottls*) with a single shared rfq.TLSCliConfig embedded under experimental.rfq.tls.*, consumed by both the price oracle and portfolio pilot clients. Macaroon paths remain per-service since macaroons are per-service identity tokens. Also collapses the two getPriceOracleTLSConfig / getPortfolioPilotTLS- Config helpers in tapcfg into one getRfqTLSConfig, drops the obsolete default* constants for fields that are already zero-valued, and adds CleanAndExpandPath for the portfolio pilot macaroon path (which was missing the same treatment as the oracle macaroon path). itest call sites updated to the new flag name.
Two small cleanups: * Swap grpc.Dial (deprecated) for grpc.NewClient in the price oracle client, matching the portfolio pilot client. * Reject macaroon paths set without a corresponding real service address (oracle: empty or mock; pilot: empty), since the macaroon would be silently read and never used.
jtobin
added
mainnet preparedness
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request harmonizes the transport and authentication settings for RFQ-related gRPC services. By unifying the TLS configuration and extending macaroon authentication to the portfolio pilot, the changes improve consistency and security across the RFQ stack. The update also includes necessary adjustments to integration tests and documentation to reflect the new configuration structure. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
(Sorry @djkazic, accidental collateral tag.) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extends TLS and macaroon authentication to the external portfolio pilot client and unifies the TLS configuration across both RFQ gRPC clients (price oracle and portfolio pilot) under a single shared experimental.rfq.tls.* namespace. One review comment was provided regarding a potential nil pointer dereference when configuring transport credentials for the portfolio pilot client.
darioAnongba
left a comment
darioAnongba
left a comment
There was a problem hiding this comment.
Approved with nits.
github-project-automation
Bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
A macaroon is a bearer token, so sending it over a connection where the server certificate isn't verified means anyone who can MITM the connection (and present any cert, including self-signed) can capture it. Tighten the validation to reject macaroon paths when either tls.disable or tls.insecure is set.
grpc.NewClient (which both RFQ clients now use) is stricter about address parsing than the legacy grpc.Dial. The fmt.Sprintf form \"%s:%s\" produces e.g. \"::1:8080\" for IPv6 literals, which the modern resolver registry can mis-parse. net.JoinHostPort wraps IPv6 addresses in brackets per RFC 3986 (\"[::1]:8080\") and is a no-op for IPv4/hostnames.
github-project-automation
Bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
2 participants
Mostly adds configurable TLS certificate verification to the portfolio pilot, bringing it to parity with the price oracle, but also smooths over a few loose ends:
First, it seems righteous to have a single RFQ TLS config, so I folded the price oracle and portfolio pilot's TLS configs together into a single RFQ TLS config. So instead of separate flags, à la:
we just deal with a single one:
Second, I added basic macaroon auth to portfolio pilot, again bringing it up to scratch with what we support for price oracles. But I didn't fold these together, as it seems correct for each service to have its own macaroon auth.
Also includes some misc. cleanup of existing minor issues (e.g. switching away from that deprecated gRPC call). Note also that I'll harmonize the release note with the cleaned-up, feature-first notes I have on the v0.8 branch; just want it in a standard form here.