[client] Support user added CAs in android client - #6941
Open
prixeus wants to merge 1 commit into
Open
Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughA global X.509 certificate pool is added with setter and cloned getter APIs. Android PEM certificates can update the pool, while client, authentication, proxy, and relay TLS configurations now consume it. ChangesGlobal TLS certificate handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Android
participant CertificatePool
participant TLSClients
Android->>CertificatePool: SetAndroidCertificates(pemCerts)
CertificatePool->>CertificatePool: Append PEM certificates
CertificatePool-->>Android: Return success or parse error
TLSClients->>CertificatePool: GetGlobalCertPool()
CertificatePool-->>TLSClients: Return cloned pool
TLSClients->>TLSClients: Configure TLS RootCAs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
client/internal/auth/pkce_flow.go (1)
202-205: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove or wire up this unused TLS configuration.
startServercallsListenAndServeat Line 256, so thisTLSConfigis never used.RootCAsalso affects server-side client-certificate verification, not outbound OAuth requests. UseListenAndServeTLSif an HTTPS callback is intended; otherwise remove this field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/auth/pkce_flow.go` around lines 202 - 205, Update the server setup in startServer to remove the unused TLSConfig and its RootCAs configuration if the callback remains HTTP; alternatively, switch the server startup from ListenAndServe to ListenAndServeTLS and provide the required certificate and key when HTTPS callbacks are intended. Keep the implementation consistent between the server configuration and startup method.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/android/certpool.go`:
- Around line 12-23: Update the certificate-pool initialization around
x509.SystemCertPool so that when the system pool is unavailable or nil, it
starts from embeddedroots.Get() instead of a new empty pool. Keep appending
Android PEM certificates and publishing the resulting pool through
util.SetGlobalCertPool, preserving the existing system-pool path.
In `@client/grpc/dialer.go`:
- Around line 35-37: Make util.GetGlobalCertPool nil-safe by checking
globalCertPool before calling Clone and returning nil when no system pool is
available. Preserve the existing nil-handling and embedded-certificate fallback
behavior at client/grpc/dialer.go:35-37, flow/client/client.go:52-54,
proxy/server.go:790-791, shared/relay/client/dialer/ws/ws.go:87-89,
shared/relay/tls/client_dev.go:15-17, and shared/relay/tls/client_prod.go:15-17;
these callers require no direct changes.
In `@client/internal/auth/pkce_flow.go`:
- Line 236: Update handleRequest’s token-exchange setup to preserve the
mTLS-configured client placed in oauth2.HTTPClient by startServer, rather than
unconditionally replacing it before Exchange. When creating a transport for the
no-client-certificate path, clone http.DefaultTransport so proxy and other
defaults remain intact, then apply RootCAs using util.GetGlobalCertPool.
In `@util/certpool.go`:
- Around line 21-24: Update GetGlobalCertPool to handle a nil globalCertPool
before calling Clone: return nil when no system certificate pool is available,
otherwise preserve the existing cloned-pool behavior so callers can use their
embedded-root fallback.
---
Nitpick comments:
In `@client/internal/auth/pkce_flow.go`:
- Around line 202-205: Update the server setup in startServer to remove the
unused TLSConfig and its RootCAs configuration if the callback remains HTTP;
alternatively, switch the server startup from ListenAndServe to
ListenAndServeTLS and provide the required certificate and key when HTTPS
callbacks are intended. Keep the implementation consistent between the server
configuration and startup method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a4ef9505-e798-476b-a0ca-e5f5c9cae88c
📒 Files selected for processing (10)
client/android/certpool.goclient/grpc/dialer.goclient/internal/auth/device_flow.goclient/internal/auth/pkce_flow.goflow/client/client.goproxy/server.goshared/relay/client/dialer/ws/ws.goshared/relay/tls/client_dev.goshared/relay/tls/client_prod.goutil/certpool.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/android/certpool.go`:
- Around line 10-14: Update SetAndroidCertificates to check whether
util.GetGlobalCertPool() returns nil before calling AppendCertsFromPEM. Add the
appropriate fallback or error return so nil pools are handled safely and invalid
Android certificates cannot trigger a panic, while preserving the existing
parsing error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f0b31943-eb89-4e1b-a5f6-e849e5d296ff
📒 Files selected for processing (10)
client/android/certpool.goclient/grpc/dialer.goclient/internal/auth/device_flow.goclient/internal/auth/pkce_flow.goflow/client/client.goproxy/server.goshared/relay/client/dialer/ws/ws.goshared/relay/tls/client_dev.goshared/relay/tls/client_prod.goutil/certpool.go
🚧 Files skipped from review as they are similar to previous changes (2)
- util/certpool.go
- flow/client/client.go
Comment on lines
+10
to
+14
| func SetAndroidCertificates(pemCerts []byte) error { | ||
| pool := util.GetGlobalCertPool() | ||
|
|
||
| if ok := pool.AppendCertsFromPEM(pemCerts); !ok { | ||
| return fmt.Errorf("failed to parse any certificates from android") |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'certpool\.go$|util' client | sed 's#^\./##' | head -100
echo "== android certpool =="
if [ -f client/android/certpool.go ]; then
cat -n client/android/certpool.go
fi
echo "== search GetGlobalCertPool/SetGlobalCertPool/pool usages =="
rg -n "GetGlobalCertPool|SetGlobalCertPool|global.*CertPool|CertPool|SetAndroidCertificates|AppendCertsFromPEM" client -S
echo "== relevant util file snippets =="
rg -n "GetGlobalCertPool|SetGlobalCertPool" -l . | while read -r f; do
echo "--- $f"
rg -n -C 8 "GetGlobalCertPool|SetGlobalCertPool" "$f" || true
doneRepository: netbirdio/netbird
Length of output: 9055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
paths=('client/android/certpool.go','client/util/certpool.go')
for path in paths:
p=Path(path)
print(f'FILE {path} exists={p.exists()}')
if not p.exists():
continue
s=p.read_text()
for name in ["GetGlobalCertPool","SetGlobalCertPool","SetAndroidCertificates"]:
print(f"\n-- {name} in {path}")
for i,line in enumerate(s.splitlines(),1):
if name in line or 'return ' in line or 'AppendCertsFromPEM' in line or 'GetSystemCertificatePool' in line:
print(f"{i}: {line}")
PYRepository: netbirdio/netbird
Length of output: 1049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== util/certpool.go =="
cat -n util/certpool.go
echo "== android/certpool.go =="
cat -n client/android/certpool.go
echo "== SetAndroidCertificates call sites =="
rg -n "SetAndroidCertificates\\(" -S .Repository: netbirdio/netbird
Length of output: 1929
Guard against GetGlobalCertpool() returning nil.
util.GetGlobalCertPool() can still return nil, and SetAndroidCertificates dereferences it unconditionally on line 13. Add a nil check/fallback before AppendCertsFromPEM so invalid Android certificate handling cannot panic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/android/certpool.go` around lines 10 - 14, Update
SetAndroidCertificates to check whether util.GetGlobalCertPool() returns nil
before calling AppendCertsFromPEM. Add the appropriate fallback or error return
so nil pools are handled safely and invalid Android certificates cannot trigger
a panic, while preserving the existing parsing error behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@util/certpool.go`:
- Around line 25-26: Update SetGlobalCertPool to validate newCertPool before
assigning globalCertPool; when the input is nil, return without modifying the
existing pool (or report the invalid caller), while preserving normal assignment
for non-nil pools.
- Line 11: Synchronize global certificate-pool access by adding an RWMutex
around the shared pool: update SetGlobalCertPool to lock while replacing the
value, and update GetGlobalCertPool to use a read lock while retrieving or
cloning it. Ensure SetAndroidCertificates cannot mutate or publish the pool
concurrently with TLS consumers such as CreateConnection and the PKCE flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bfc45671-40d9-47b3-a910-53e597582913
📒 Files selected for processing (10)
client/android/certpool.goclient/grpc/dialer.goclient/internal/auth/device_flow.goclient/internal/auth/pkce_flow.goflow/client/client.goproxy/server.goshared/relay/client/dialer/ws/ws.goshared/relay/tls/client_dev.goshared/relay/tls/client_prod.goutil/certpool.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/android/certpool.go
- flow/client/client.go
- shared/relay/client/dialer/ws/ws.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/internal/auth/pkce_flow.go (1)
202-205: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove the ineffective server-side
RootCAsconfiguration.
startServerlaunches this server withListenAndServe(), notListenAndServeTLS(), soTLSConfig.RootCAsis never used. It does not affect the outbound OAuth exchange, which is configured separately at lines 282-295. Remove it, or switch the callback to explicit TLS serving if that is intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/auth/pkce_flow.go` around lines 202 - 205, Update the server construction in startServer to remove the ineffective TLSConfig.RootCAs setting, since the callback uses ListenAndServe rather than TLS serving; leave the separate outbound OAuth TLS configuration unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@util/certpool.go`:
- Around line 48-50: Update the exported documentation comment immediately above
GetSystemCertPool so it begins with the exact function name and accurately
describes the function’s system CA certificate behavior.
- Around line 32-36: SetGlobalCertPool must clone the provided pool before
publishing it globally. Within SetGlobalCertPool, clone newCertPool after the
nil check, then store the clone under the existing guard lock so later mutations
to the caller’s pool cannot affect globalCertPool.
---
Nitpick comments:
In `@client/internal/auth/pkce_flow.go`:
- Around line 202-205: Update the server construction in startServer to remove
the ineffective TLSConfig.RootCAs setting, since the callback uses
ListenAndServe rather than TLS serving; leave the separate outbound OAuth TLS
configuration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 576adf74-b8ee-4f83-b79e-a78ec2afc34d
📒 Files selected for processing (10)
client/android/certpool.goclient/grpc/dialer.goclient/internal/auth/device_flow.goclient/internal/auth/pkce_flow.goflow/client/client.goproxy/server.goshared/relay/client/dialer/ws/ws.goshared/relay/tls/client_dev.goshared/relay/tls/client_prod.goutil/certpool.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/android/certpool.go
- flow/client/client.go
- shared/relay/client/dialer/ws/ws.go
The android stores the Certificate Authorities separately which are installed by the user. The Golang ecosystem does not care about them by default, so there was no possibility to use them in self-hosted environment on android clients with user supplied CA. As my understanding to fix that the android code should collect the Certificates Authorities with java API, supply them to go code, and consume it combined with the system provided CAs. By default the system CAs will be used in Golang.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The android stores the Certificate Authorities separately which are installed by the user. The Golang ecosystem does not care about them by default, so there was no possibility to use them in self-hosted environment on android clients with user supplied CA.
As my understanding to fix that the android code should collect the Certificates Authorities with java API, supply them to go code, and consume it combined with the system provided CAs.
By default the system CAs will be used in Golang.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Added some in-line comments in go code
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit