Skip to content

[Docs][Modernize] Add a review document talking about the codebase #165

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.

Sign up for GitHub

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

Open
morrowc wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[Docs][Modernize] Add a review document talking about the codebase #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf7c182
[Docs][Modernize] Add a review docuemnt talking about the codebase an…
May 29, 2026
d9df4a9
go mod tidy outputs.
May 29, 2026
b6e0796
[Docs]Add a modernization plan to the docs collected for this review/…
May 29, 2026
bd7722b
Move the modeernization plan to proper place.
May 31, 2026
755dbb8
modernize: Implement Phase 1 (Build System & Dependency Modernization)
May 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 5 additions & 88 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,103 +1,22 @@
TRIDENT := ${PWD}
GOPATH := ${PWD}/ext/_gopath
export GOPATH

ifndef $GIT_BRANCH
GIT_BRANCH := `git rev-parse --abbrev-ref HEAD 2>/dev/null || printf "nogit"`
GIT_BRANCH := $(shell git rev-parse --abbrev-ref HEAD 2>/dev/null || printf "nogit")
endif

# Pull correct branches for PF from local files.
PF_GIT_BRANCH = master
PF_GIT_REPO = https://github.com/tridentli/pitchfork.git

# dpkg-buildpackage calls make, so <all> should be empty.
all:
@echo "All does nothing"

help:
@echo "all - Build it all (called from dpkg-buildpackage)"
@echo "clean_ext - cleans 'ext' directory"
@echo "pkg - ext+deps+pkg_only"
@echo "pkg_only - Only builds Debian package (no dependency updates)"
@echo "check - runs: go vet/fmt"
@echo "tests - Runs all Golang based tests"
@echo "vtests - Runs all Golang based tests (verbose)"
@echo "versions - Show pre-selected versions"
@echo "ext - Fetches initial external dependencies (for use by local devs or trident-ext-pkg)"
@echo "deps - Updates external dependencies (for use by local devs or trident-ext-pkg)"
@echo "build_ext - Create a GOPATH version of the local trident dir"
@echo "check - runs: go vet/fmt, also on Pitchfork"
@echo "tests - Runs all Golang based tests"
@echo "vtests - Runs all Golang based tests (verbose)"

clean_ext:
@echo "Cleansing 'ext'"
@rm -rf ext

pkg: ext deps pkg_only

pkg_only: build_ext
@echo "Starting package build..."
dpkg-buildpackage -uc -us -F
@echo "Starting package build - done"

versions:
@echo "Trident: $(GIT_BRANCH)"
@echo "Pitchfork: $(PF_GIT_BRANCH)"
@echo
@printf "Go version: "; go version
@printf "Trident branch: "; git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "nogit"; printf "Trident Git commit: "; git rev-parse HEAD 2>/dev/null || echo "nogit"
@echo
@printf "Build OS: "
@if [ -f /etc/debian_version ]; then printf "Debian "; cat /etc/debian_version; else echo "Not Debian"; fi
@echo

build_ext:
@echo "Creating temporary 'ext' for build..."

@echo
@echo "Creating 'ext' directory for external dependencies and gopath"
@mkdir -p ext/_gopath/src/trident.li
@echo "Symlink Trident into GOPATH"
@ln -s ../../../../ ext/_gopath/src/trident.li/trident

ext:
@$(MAKE) versions
@echo "Retrieving 'ext' dependencies..."

@echo "Git Clone Pitchfork into GOPATH [ $(PF_GIT_BRANCH) ]"
@mkdir -p ext/_gopath/src/trident.li
@git clone -b $(PF_GIT_BRANCH) $(PF_GIT_REPO) ext/_gopath/src/trident.li/pitchfork

@echo
@echo "Clone Trident Go Dependencies"
@mkdir -p ext/_gopath/src/trident.li
@git clone https://github.com/tridentli/go ext/_gopath/src/trident.li/go

@echo
@echo "Retrieving 'ext' dependencies - done"
@echo

deps: ext
@$(MAKE) versions
@echo "Updating 'ext' Dependencies..."

@echo
@echo "Updating Pitchfork..."
@cd ext/_gopath/src/trident.li/pitchfork; (git pull 2>/dev/null && git checkout ${PF_GIT_BRANCH} && printf "Pitchfork branch: " && git rev-parse --abbrev-ref HEAD && printf "Pitchfork GIT commit: " && git rev-parse HEAD) || echo "NO GIT"

@echo
@echo "Updating Pitchfork dependencies"
@cd ext/_gopath/src/trident.li/pitchfork && $(MAKE) deps

@echo
@echo "Updating Trident Go Dependencies"
@cd ext/_gopath/src/trident.li/go; git pull 2>/dev/null || echo "NO GIT"

@echo
@echo "Updating Golang dependencies (imports)..."
@GOPATH=${PWD}/ext/_gopath go get -v -d -t ./...

@echo
@echo "Updating Dependencies - done"
@echo

check:
@echo "Running 'go vet'"
Expand All @@ -108,11 +27,9 @@ check:
tests:
@echo "Running 'go test'..."
@go test ./...
@cd ext/_gopath/src/trident.li/pitchfork && make tests

vtests:
@echo "Running 'go test' (verbose)..."
@go test -v ./...
@cd ext/_gopath/src/trident.li/pitchfork && make vtests

.PHONY : all help clean_ext pkg pkg_only versions deps check tests vtests
.PHONY : all help check tests vtests versions
113 changes: 113 additions & 0 deletions doc/modernization/modernization_plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# Phased Modernization and Security Review Plan for Trident

This document outlines a phased plan to modernize the Trident codebase, transition it to modern Go standards, establish a robust testing harness with high code coverage, and conduct a thorough security review.

---

## Phase 1: Build System & Dependency Modernization (Completed)

**Objective**: Transition the codebase fully to modern Go Modules, eliminating legacy `GOPATH` requirements and ensuring the project can be built and tested using standard Go toolchain commands.

### 1.1. Go Module Transition & Clean Up
* **Issue**: The current build system relies on a legacy `GOPATH` hack (`ext/_gopath`) and clones dependencies manually via Git in the `Makefile`.
* **Tasks**:
* [x] Permanently adopt Go Modules (`go.mod`).
* [x] Update the `go.mod` file to use a modern Go version (e.g., `go 1.21` or higher) to leverage modern language features and security updates. (Updated to Go 1.26.2).
* [x] Configure `replace` directives in `go.mod` to point to local checkouts ... or migrate them to standard GitHub import paths. (Migrated to GitHub import paths: `github.com/tridentli/pitchfork`, `github.com/tridentli/go`, and `github.com/secludedsoc/keyval` to fully eliminate local checkout requirements).
* [x] Ensure all local dependency repositories have valid, tidied `go.mod` files. (N/A: bypassed by using GitHub replaces).
* [x] Clean up the `Makefile` to remove `GOPATH` exports and replace legacy commands with standard `go build`, `go test`, and `go vet` commands.

### 1.2. Code Quality & Static Analysis (Style Fixes)
* **Issue**: `go vet` currently flags multiple warnings regarding unkeyed struct literals in menu definitions (e.g., `PfMEntry` and `PfUIMentry`).
* **Tasks**:
* [x] Refactor all struct initializations to use keyed fields.
* [x] Integrate `go vet` and `go fmt` checks as blocking steps in the local development loop. (Integrated into Makefile `check` target).

---

## Phase 2: Test Harness & Unit Testing Strategy

**Objective**: Introduce a comprehensive test suite targeting near 100% code coverage for core business logic in `src/lib/`, and establish an automated test execution environment.

### 2.1. Establish the Test Harness
* **Database Dependency**: Trident relies heavily on PostgreSQL. Running unit tests requires a strategy to handle database operations without polluting production or requiring a complex local DB setup for every developer.
* **Proposed Solutions**:
1. **Unit Testing with Mocking**: Use `github.com/DATA-DOG/go-sqlmock` to mock database responses for unit tests in `src/lib/` (e.g., testing `user.go`, `vouch.go` logic without a real DB). This is fast and suitable for pure unit tests.
2. **Integration Testing with Testcontainers**: Use `github.com/testcontainers/testcontainers-go` to automatically spin up a temporary PostgreSQL Docker container during test execution. This allows running integration tests against a real database with the actual schema applied, ensuring query compatibility.
* **Decision**: We will implement **both**: `go-sqlmock` for rapid unit tests of business logic, and `testcontainers` for database-centric integration tests.

### 2.2. Unit Testing Targets (`src/lib/`)
* **User Management (`src/lib/user.go`)**:
* Test `IsNominator` and `BestNominator` with mocked DB states.
* Test `user_pw_send` (split password recovery): Verify that the recovery token is correctly generated, split into two unique halves, hashed/stored correctly, and that the correct emails are dispatched to the user and nominator.
* Test `user_merge` transactional integrity.
* **Vouching Registry (`src/lib/vouch.go`)**:
* Test vouch validation rules: Ensure a user cannot vouch for themselves, cannot vouch without meeting group attestations, and that positive/negative vouches transition member states correctly.
* **Group Attestations (`src/lib/group_attestation.go`)**:
* Test parsing and validation of custom group attestations.

### 2.3. UI Testing (`src/ui/`)
* * Use `net/http/httptest` to mock HTTP requests and responses.
* Verify that UI handlers correctly parse form inputs, enforce session permissions, and call the underlying library functions.
* Verify that HTML outputs are correctly sanitized using `Blue Monday`.

### 2.4. CI/CD Integration (GitHub Actions)
* Define a GitHub Actions workflow (`.github/workflows/test.yml`) that:
1. Runs on every push and Pull Request.
2. Sets up a modern Go environment.
3. Starts a PostgreSQL service container (or relies on Testcontainers).
4. Runs `go vet ./...` and `go fmt` checks.
5. Runs all tests with race detection enabled: `go test -race -coverprofile=coverage.out ./...`.
6. Reports code coverage metrics.

---

## Phase 3: Code Refactoring & Idiomatic Go Adoption

**Objective**: Clean up legacy code patterns, improve safety, and adopt modern Go idioms.

### 3.1. Language Feature Upgrades (Go 1.21+)
* **Structured Logging**: Replace custom verbose logging/debugging with the standard `log/slog` package for structured, levels-based logging.
* **Slices & Maps**: Replace manual slice manipulation loops with functions from the standard `slices` and `maps` packages.
* **Context Propagation**: Ensure `context.Context` is passed through all database and network boundary calls (e.g., propagating context from HTTP requests down to `pgx` database queries) to support cancellation and timeouts.

### 3.2. Refactoring Package Names and Cycle Resolution
* Resolve the duplicate `crypt` package naming (in `osutil-crypt` and its `common` subdirectory) by renaming the internal packages to be distinct (e.g., `common` to `cryptcommon` or merging them) to prevent import shadowing and confusion.
* Ensure explicit error wrapping using `fmt.Errorf("...: %w", err)` to preserve error chains for debugging.

---

## Phase 4: Deep Security Review Plan & Findings Mitigation

**Objective**: Conduct a rigorous security assessment of Trident's high-trust model and establish a clear protocol for addressing vulnerabilities.

### 4.1. Static & Dynamic Security Scanning
* **SAST (Static Application Security Testing)**:
* Integrate `github.com/securego/gosec/v2` into the CI/CD pipeline to scan for Go-specific security issues (e.g., hardcoded credentials, weak cryptography, unsafe SQL query construction).
* **Vulnerability Scanning**:
* Use `govulncheck` to identify known vulnerabilities in imported dependencies.

### 4.2. Targeted Manual Security Audits
Given Trident's threat model (high-trust threat intelligence exchange), we must manually audit these critical areas:

1. **Split Password Recovery Flow**:
* **Entropy**: Verify that the random generator used for `user_portion` and `nom_portion` (`pw.GenPass`) uses a cryptographically secure source (`crypto/rand`) and has sufficient entropy.
* **Storage**: Ensure the combined hash stored in the DB uses a modern secure hashing algorithm (e.g., bcrypt, Argon2id) and is not vulnerable to timing attacks during comparison.
* **Transit**: Verify that the transit channels (emails) are encrypted (enforced STARTTLS in Postfix configuration).
2. **SQL Injection Prevention**:
* Review all raw SQL strings in `src/lib/` and `pitchfork`. Ensure *all* queries use parameterized placeholders (`$1`, `$2`) and that no string concatenation of user input occurs.
3. **Cross-Site Scripting (XSS) in Wiki & UI**:
* Review the rendering pipeline of the Wiki. Ensure that `Blue Monday` sanitization is applied *immediately before* rendering to the user and that no unescaped templates allow bypasses.
* Audit the dual Markdown engine setup to ensure no discrepancies allow "mutation XSS" (where preview behaves differently from final rendered output in a exploitable way).
4. **Session & Authentication (JWT)**:
* Review JWT signing key generation and storage. Ensure keys are loaded securely and rotated.
* Verify JWT expiration times and revocation mechanisms (if any).
* Ensure secure cookie flags (`Secure`, `HttpOnly`, `SameSite`) are enforced in production (disabling them only via explicitly flagged development arguments like `--insecurecookies`).

### 4.3. Handling Security Findings
* **Triage Process**: Findings from scans and audits will be logged in a secure, restricted tracker and triaged by severity (Critical, High, Medium, Low).
* **Remediation SLA**:
* *Critical/High*: Fix within 14 days.
* *Medium*: Fix within 30 days.
* *Low*: Fix within 90 days.
* **Coordinated Disclosure**: Since Trident is a community project, establish a secure security contact (currently `project@trident.li`) and a policy for coordinated disclosure, allowing existing operators of Trident instances to update before details are made public.
Loading