feat(security): implement certificate pinning and reorganize project scripts#40
feat(security): implement certificate pinning and reorganize project scripts#40
Conversation
WalkthroughAdds real Coinbase certificate pins and enables enforcement, changes CertificatePinner API, introduces unit tests and a pin-extraction script, hardens shell scripts, switches runner scripts to array-style arg handling and updates docs to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant TLS as Remote TLS Server
participant SSL as OpenSSL
participant Pin as CertificatePinner
App->>TLS: TCP connect + TLS handshake
TLS-->>SSL: Server certificate chain
App->>SSL: obtain peer X509 certificate
App->>Pin: verifyCertificate(X509 cert)
Note right of Pin #DDEBF7: compute SPKI SHA-256 (base64)
Pin-->>App: result (match / no-match) + pinning enabled?
alt pinning enabled and match
App-->>App: proceed
else pinning enabled and no match
App-->>App: abort connection
else pinning disabled
App-->>App: proceed (skip pin check)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
chizy7
changed the title
chizy7
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/cleanup.sh (1)
50-58: Handle empty/missing build dir before callingdu
du -h build/* …(anddu -sh build) exits non‑zero when the directory is empty or missing; withset -euo pipefailthat terminates option 5 outright. Please guard these calls so the menu keeps running even when there’s nothing to report.- echo "Build directory contents:" - du -h build/* 2>/dev/null | sort -hr | head -10 + echo "Build directory contents:" + if compgen -G "build/*" > /dev/null; then + du -h build/* 2>/dev/null | sort -hr | head -10 + else + echo "(build directory empty)" + fi echo "" echo "Total build directory size:" - du -sh build + if [[ -d build ]]; then + du -sh build + else + echo "build directory does not exist" + fi
🧹 Nitpick comments (9)
scripts/extract_cert_pin.sh (2)
5-5: Harden shell safety (errexit alone is brittle).Enable nounset and pipefail, and add an error trap for debuggability.
-set -e +set -Eeuo pipefail +trap 'echo "[ERROR] line $LINENO: command failed"; exit 1' ERR + +# Dependencies +command -v openssl >/dev/null 2>&1 || { echo "openssl not found (install OpenSSL)"; exit 1; }
36-41: Avoid set -e pitfalls around command substitution; be resilient to handshake failures.If s_client/x509 fails, set -e can abort the script. Make the substitution tolerant so the later empty-check works.
- if [ -n "$TIMEOUT_CMD" ]; then - CERT_CHAIN=$($TIMEOUT_CMD openssl s_client -connect "$ENDPOINT" -servername "$HOST" </dev/null 2>/dev/null | openssl x509 -outform PEM) - else - # Fallback without timeout - CERT_CHAIN=$(openssl s_client -connect "$ENDPOINT" -servername "$HOST" </dev/null 2>/dev/null | openssl x509 -outform PEM) - fi + if [ -n "$TIMEOUT_CMD" ]; then + CERT_CHAIN=$($TIMEOUT_CMD openssl s_client -connect "$ENDPOINT" -servername "$HOST" </dev/null 2>/dev/null | openssl x509 -outform PEM || true) + else + # Fallback without timeout + CERT_CHAIN=$(openssl s_client -connect "$ENDPOINT" -servername "$HOST" </dev/null 2>/dev/null | openssl x509 -outform PEM || true) + fiscripts/run-docker.sh (1)
155-201: Array-based args: solid and safe; minor polish optional.Using DOCKER_ARGS as an array and expanding with "${DOCKER_ARGS[@]}" prevents word-splitting bugs. Looks good.
- Consider adding --pull=missing to build paths to keep images fresh in CI.
- For live mode, you may want to pass --init to improve signal handling for the app in Docker.
scripts/run-native.sh (1)
296-333: Switch to array-style ARGS is correct.Safer arg passing and consistent with Docker script. Invocation with "${ARGS[@]}" is the right pattern.
You can add set -o pipefail (and optionally -u) at the top for stronger safety, mirroring the extract script.
core/utils/CertificatePinner.cpp (2)
155-164: Avoid constructing a new CertificatePinner inside getCertificateFingerprint.Creating a fresh instance triggers initializeDefaultPins() and logs every call. Use helpers directly to avoid side effects.
If this method is non-static, simplest fix:
std::string CertificatePinner::getCertificateFingerprint(X509* cert) { unsigned char hash[SHA256_DIGEST_LENGTH]; - - CertificatePinner pinner; - if (!pinner.extractPublicKeyHash(cert, hash)) { + if (!extractPublicKeyHash(cert, hash)) { return ""; } - - return pinner.hashToBase64(hash, SHA256_DIGEST_LENGTH); + return hashToBase64(hash, SHA256_DIGEST_LENGTH); }If it’s a static method, consider making extractPublicKeyHash and hashToBase64 static as well, or move hashing/encoding to free functions to avoid instantiation.
190-211: Ensure SHA256 is declared; include <openssl/sha.h>.SHA256() requires <openssl/sha.h>. If it’s not included via the header, builds will fail with newer toolchains.
- Add this include near the top of the file:
#include <openssl/sha.h>
- Minor: check lengths explicitly and guard against negative returns from i2d_PUBKEY (already partly handled). Current memory management (EVP_PKEY_free / OPENSSL_free) is correct.
Based on learningsdocs/security/CERTIFICATE_PINNING.md (1)
146-156: Specify language for fenced code block.The expected test output should have a language identifier for proper syntax highlighting.
As per static analysis tools
Apply this diff:
-``` +```text [==========] Running 6 tests from 1 test suite. ... === Testing Real Coinbase Certificate Pinning ===tests/unit/CertificatePinnerTest.cpp (2)
48-54: Consider logging setsockopt failures.While timeouts are best-effort, silently ignoring setsockopt failures could hide socket configuration issues during testing.
Consider adding a warning:
if (setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)) < 0 || setsockopt(sock, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)) < 0) { - // Continue anyway - timeouts are best-effort - // But log that timeout setting failed + std::cerr << "Warning: Failed to set socket timeout" << std::endl; }
103-214: Consider additional test coverage.The current test suite covers basic functionality well. Consider adding tests for:
- Multiple pins per endpoint (backup pins during rotation)
- Pin update workflow
- Pin expiry date validation
- Configuration file loading (
loadPinsFromFile)These tests would align with the best practices documented in CERTIFICATE_PINNING.md.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)README.md(11 hunks)core/utils/CertificatePinner.cpp(1 hunks)docs/security/CERTIFICATE_PINNING.md(1 hunks)docs/user_guide/getting_started.md(2 hunks)scripts/cleanup.sh(3 hunks)scripts/extract_cert_pin.sh(1 hunks)scripts/run-docker.sh(3 hunks)scripts/run-native.sh(3 hunks)tests/unit/CertificatePinnerTest.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/run-docker.sh (1)
scripts/run-native.sh (1)
print_info(23-25)
tests/unit/CertificatePinnerTest.cpp (1)
core/utils/CertificatePinner.cpp (2)
getCertificateFingerprint(155-164)getCertificateFingerprint(155-155)
🪛 markdownlint-cli2 (0.18.1)
docs/security/CERTIFICATE_PINNING.md
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Test (macos-latest, Release)
- GitHub Check: Build and Test (ubuntu-latest, Debug)
- GitHub Check: Build and Test (ubuntu-latest, Release)
- GitHub Check: Security Scan
🔇 Additional comments (7)
scripts/extract_cert_pin.sh (1)
49-51: Note: This computes SPKI (pin-sha256) correctly.Good use of x509 -pubkey + DER + sha256 + base64.
Please confirm these pins are compared against SPKI hashes in code (not full cert fingerprints). If so, we're aligned.
docs/user_guide/getting_started.md (1)
86-86: Path updates look correct.Referencing scripts/run-native.sh aligns with the repo reorg.
Also applies to: 210-210
docs/security/CERTIFICATE_PINNING.md (1)
1-290: Excellent comprehensive documentation.This certificate pinning guide is thorough and well-structured, covering all essential aspects: configuration, testing, troubleshooting, best practices, security considerations, and API reference. The inclusion of real Coinbase pins with expiry dates and clear update procedures demonstrates production-readiness.
README.md (2)
107-112: LGTM: Consistent script path reorganization.All script references have been systematically updated from
./run-*.shtoscripts/run-*.sh, correctly reflecting the new directory structure. The changes are complete and consistent across Quick Start, Manual Building, Docker Deployment, and Script Documentation sections.Also applies to: 117-121, 131-131, 143-143, 148-149, 159-160, 190-194, 295-295, 309-309, 332-356, 366-381, 395-401
543-543: Good addition of Certificate Pinning Guide.The new security documentation link is appropriately placed in the System Administration section alongside other security-related guides.
tests/unit/CertificatePinnerTest.cpp (1)
117-160: Well-designed real endpoint validation test.This test properly validates certificate pinning against a live Coinbase endpoint with appropriate error handling (GTEST_SKIP) for network unavailability and detailed diagnostic output for debugging.
CMakeLists.txt (1)
412-422: No duplicate certificate_pinner_tests definition
Targetcertificate_pinner_testsis defined only once in CMakeLists.txt.Likely an incorrect or invalid review comment.
| void CertificatePinner::initializeDefaultPins() { | ||
| // Coinbase Pro pins (note to self:just an example - these should be updated | ||
| // with real pins) | ||
| // Coinbase certificate pins (extracted on 2025-10-13) | ||
| // Certificate valid until: Dec 22 02:31:45 2025 GMT | ||
| // Issuer: Google Trust Services (WE1) | ||
|
|
||
| // Primary WebSocket endpoint for Coinbase Pro/Advanced Trade | ||
| addPin("ws-feed.exchange.coinbase.com", | ||
| "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=", false); | ||
| "mpzb4t3w5gAFZJGODlP0+FJa+wjD/bOQszdCDs6BTmU=", true); | ||
|
|
||
| // Kraken pins | ||
| addPin("ws.kraken.com", "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB=", false); | ||
| // Coinbase Prime WebSocket endpoint | ||
| addPin("ws-feed.prime.coinbase.com", | ||
| "ERzVGmVjfqDVEe2YEp5l1B7zaXEJoSYinwL9InU8Pis=", true); | ||
|
|
||
| // Binance pins | ||
| addPin("stream.binance.com", | ||
| "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC=", false); | ||
| // Advanced Trade WebSocket endpoint | ||
| addPin("advanced-trade-ws.coinbase.com", | ||
| "Is81uMxmmDbwnPDQSpN+FgZ5nfv2XenZ8Ql8zE4Vbzs=", true); | ||
|
|
||
| // TODO and note to remind myself to update these pins | ||
| // Self Note: In production, these should be real certificate pins | ||
| // obtained by connecting to the services and extracting public key hashes | ||
| spdlog::info("Initialized default certificate pins (example pins - update " | ||
| "for production)"); | ||
| // Note: Certificate pinning is now enabled with real pins | ||
| // Pins should be updated before certificate expiry (Dec 2025) | ||
| // To extract new pins, run: scripts/extract_cert_pin.sh | ||
| spdlog::info("Initialized certificate pins for Coinbase (pinning enabled)"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ship with multiple pins per host (primary + backup) to tolerate key rotation.
Single-SPKI pin per hostname risks production outages when exchanges rotate keys. Add at least one backup pin (documented) per endpoint.
Example (pattern only):
// Primary
addPin("ws-feed.exchange.coinbase.com", "<primary_base64_spki>", true);
// Backup (next cert/key already published or staged)
addPin("ws-feed.exchange.coinbase.com", "<backup_base64_spki>", true);We can help generate/update backups with scripts/extract_cert_pin.sh and staged endpoints if available.
🤖 Prompt for AI Agents
In core/utils/CertificatePinner.cpp around lines 166-187, each hostname
currently has only a single SPKI pin which risks outages during key rotation;
add at least one backup pin entry per hostname (same host string, addPin call
with the backup base64 SPKI and the same boolean flag) for all listed endpoints,
using scripts/extract_cert_pin.sh to extract the backup SPKI(s); also update the
inline comment to note primary vs backup pins and ensure backup pins are added
before the primary certificate expiry.
| echo "Cleaning build files..." | ||
| cd build && make clean | ||
| (cd build && make clean) | ||
| echo "Build files cleaned" | ||
| ;; |
There was a problem hiding this comment.
Guard make clean when build/ is absent under set -e
With set -euo pipefail, (cd build && make clean) now aborts the whole script if build/ doesn’t exist. Please gate the cleanup so we either skip with a friendly message or create the directory first.
- echo "Cleaning build files..."
- (cd build && make clean)
- echo "Build files cleaned"
+ if [[ -d build ]]; then
+ echo "Cleaning build files..."
+ (cd build && make clean)
+ echo "Build files cleaned"
+ else
+ echo "Build directory not found; nothing to clean."
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Cleaning build files..." | |
| cd build && make clean | |
| (cd build && make clean) | |
| echo "Build files cleaned" | |
| ;; | |
| if [[ -d build ]]; then | |
| echo "Cleaning build files..." | |
| (cd build && make clean) | |
| echo "Build files cleaned" | |
| else | |
| echo "Build directory not found; nothing to clean." | |
| fi | |
| ;; |
🤖 Prompt for AI Agents
In scripts/cleanup.sh around lines 18 to 21, the subshell `(cd build && make
clean)` will abort the script under `set -euo pipefail` if the build/ directory
is missing; change this to first test for the directory and only run `make
clean` inside it when present (e.g., if [ -d build ]; then (cd build && make
clean); else echo "No build directory, skipping cleanup"; fi) so the script
either skips with a friendly message or creates the directory first depending on
your chosen behavior.
| struct hostent* host = gethostbyname(hostname.c_str()); | ||
| if (!host) { | ||
| close(sock); | ||
| SSL_free(ssl); | ||
| SSL_CTX_free(ctx); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Replace deprecated gethostbyname with getaddrinfo.
gethostbyname is deprecated since POSIX.1-2008 and is not thread-safe. Use getaddrinfo instead for thread-safe hostname resolution.
Apply this diff to use the modern, thread-safe API:
- // Resolve hostname
- struct hostent* host = gethostbyname(hostname.c_str());
- if (!host) {
+ // Resolve hostname (thread-safe)
+ struct addrinfo hints = {}, *result = nullptr;
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+
+ if (getaddrinfo(hostname.c_str(), nullptr, &hints, &result) != 0 || !result) {
+ if (result) freeaddrinfo(result);
close(sock);
SSL_free(ssl);
SSL_CTX_free(ctx);
return nullptr;
}
struct sockaddr_in addr;
addr.sin_family = AF_INET;
addr.sin_port = htons(port);
- addr.sin_addr = *reinterpret_cast<struct in_addr*>(host->h_addr);
+ addr.sin_addr = reinterpret_cast<struct sockaddr_in*>(result->ai_addr)->sin_addr;
+ freeaddrinfo(result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct hostent* host = gethostbyname(hostname.c_str()); | |
| if (!host) { | |
| close(sock); | |
| SSL_free(ssl); | |
| SSL_CTX_free(ctx); | |
| return nullptr; | |
| } | |
| // Resolve hostname (thread-safe) | |
| struct addrinfo hints = {}, *result = nullptr; | |
| hints.ai_family = AF_INET; | |
| hints.ai_socktype = SOCK_STREAM; | |
| if (getaddrinfo(hostname.c_str(), nullptr, &hints, &result) != 0 || !result) { | |
| if (result) freeaddrinfo(result); | |
| close(sock); | |
| SSL_free(ssl); | |
| SSL_CTX_free(ctx); | |
| return nullptr; | |
| } | |
| struct sockaddr_in addr; | |
| addr.sin_family = AF_INET; | |
| addr.sin_port = htons(port); | |
| addr.sin_addr = reinterpret_cast<struct sockaddr_in*>(result->ai_addr)->sin_addr; | |
| freeaddrinfo(result); |
🤖 Prompt for AI Agents
In tests/unit/CertificatePinnerTest.cpp around lines 57 to 63, replace the
deprecated, non-thread-safe gethostbyname usage with getaddrinfo: prepare an
addrinfo hints struct (e.g., ai_family = AF_UNSPEC, ai_socktype = SOCK_STREAM),
call getaddrinfo(hostname.c_str(), nullptr, &hints, &res), handle errors by
cleaning up socket/SSL/ctx and returning nullptr, use the returned
addrinfo->ai_addr/ai_addrlen when creating/connecting the socket (or iterate
results until connect succeeds), and finally call freeaddrinfo(res) before
returning; ensure all existing cleanup paths remain intact.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/docker-test.yml(3 hunks).github/workflows/pr-title-lint.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Test (ubuntu-latest, Release)
- GitHub Check: Build and Test (macos-latest, Release)
- GitHub Check: Build and Test (ubuntu-latest, Debug)
- GitHub Check: Security Scan
- GitHub Check: Test Docker Scripts
🔇 Additional comments (1)
.github/workflows/pr-title-lint.yml (1)
39-40: Regex update looks great.The refined capture groups keep
headerPatternCorrespondencealigned while enforcing the subject length cap.
| - 'scripts/run-docker.sh' | ||
| - 'Dockerfile' | ||
| - '.github/workflows/docker-test.yml' | ||
| push: | ||
| branches: [ main ] | ||
| paths: | ||
| - 'run-docker.sh' | ||
| - 'scripts/run-docker.sh' | ||
| - 'Dockerfile' |
There was a problem hiding this comment.
Include scripts/cleanup.sh in workflow triggers.
This workflow manipulates scripts/cleanup.sh, but the paths filters only watch scripts/run-docker.sh and Dockerfile. If cleanup.sh changes, the job won’t run, so you’ll miss catching regressions in the cleanup logic. Add the file to both pull_request.paths and push.paths.
pull_request:
paths:
- 'scripts/run-docker.sh'
+ - 'scripts/cleanup.sh'
- 'Dockerfile'
- '.github/workflows/docker-test.yml'
push:
branches: [ main ]
paths:
- 'scripts/run-docker.sh'
+ - 'scripts/cleanup.sh'
- 'Dockerfile'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - 'scripts/run-docker.sh' | |
| - 'Dockerfile' | |
| - '.github/workflows/docker-test.yml' | |
| push: | |
| branches: [ main ] | |
| paths: | |
| - 'run-docker.sh' | |
| - 'scripts/run-docker.sh' | |
| - 'Dockerfile' | |
| pull_request: | |
| paths: | |
| - 'scripts/run-docker.sh' | |
| - 'scripts/cleanup.sh' | |
| - 'Dockerfile' | |
| - '.github/workflows/docker-test.yml' | |
| push: | |
| branches: [ main ] | |
| paths: | |
| - 'scripts/run-docker.sh' | |
| - 'scripts/cleanup.sh' | |
| - 'Dockerfile' |
🤖 Prompt for AI Agents
In .github/workflows/docker-test.yml around lines 6 to 13, the workflow's
pull_request and push path filters currently only include
'scripts/run-docker.sh' and 'Dockerfile' but omit 'scripts/cleanup.sh', so
changes to cleanup.sh won't trigger the job; update both pull_request.paths and
push.paths to also include 'scripts/cleanup.sh' so the workflow runs when that
file changes.
# [1.4.0](v1.3.1...v1.4.0) (2025-10-13) ### Features * **security:** implement certificate pinning and reorganize project scripts ([#40](#40)) ([f8aad7d](f8aad7d))
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
scripts/directoryChanges
Security Enhancements
scripts/extract_cert_pin.shfor easy pin updatestests/unit/CertificatePinnerTest.cppwith real endpoint validationProject Organization
run-native.sh,run-docker.sh, andcleanup.shtoscripts/directorySecurity Impact
Certificate pins are now enforced for production security:
Th PR closes issues #27
Summary by CodeRabbit