Skip to content

Secure Canvas API token persistence with AES-GCM and legacy token reset migration - #676

Open
pconrad with Copilot wants to merge 13 commits into
mainfrom
copilot/secure-canvas-api-keys
Open

Secure Canvas API token persistence with AES-GCM and legacy token reset migration#676
pconrad with Copilot wants to merge 13 commits into
mainfrom
copilot/secure-canvas-api-keys

Conversation

Copilot AI commented May 29, 2026

Copy link
Copy Markdown
Contributor

Canvas API keys were persisted in plaintext, creating unnecessary exposure risk for FERPA-sensitive integrations. This change encrypts tokens at rest, preserves plaintext interoperability at request boundaries, and adds a migration to clear legacy plaintext tokens from existing rows.

  • Encrypted token storage and retrieval

    • Added CanvasApiTokenSecurityService for versioned token encryption/decryption (enc:v1:) using AES-GCM.
    • Encryption key is externally configured via CANVAS_API_TOKEN_ENCRYPTION_KEY (app.canvas.api-token-encryption-key), with fail-closed behavior for invalid/missing key when encrypting.
  • Controller write/read path hardening

    • CoursesController.postCourse(...) and updateCourseWithCanvasToken(...) now encrypt incoming Canvas tokens before save.
    • getCanvasInfo(...) decrypts before applying existing masking logic, so UX behavior stays consistent while storage is ciphertext.
    • Update comparisons now use decrypted current value to avoid false “changed” detection against ciphertext.
  • Canvas API call boundary handling

    • CanvasService now decrypts token only when constructing outbound Authorization headers for Canvas GraphQL calls.
  • Schema and configuration updates

    • Added Liquibase migration to expand COURSE.CANVAS_API_TOKEN from VARCHAR(255) to VARCHAR(1024) to accommodate encrypted payloads.
    • Added Liquibase migration to set all existing COURSE.CANVAS_API_TOKEN values to NULL, removing legacy plaintext tokens so replacements are stored encrypted via the new write path.
    • Updated .env.SAMPLE and application.properties for encryption key configuration.
  • Security documentation for audit

    • Added /docs/README_Canvas_API_Keys.md documenting threat model focus, controls, key management expectations, backward compatibility, and operational guidance for security review.
// write path
course.setCanvasApiToken(canvasApiTokenSecurityService.encrypt(canvasApiToken));

// use path
.header("Authorization", "Bearer " + canvasApiTokenSecurityService.decrypt(course.getCanvasApiToken()))

Copilot AI linked an issue May 29, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Secure Canvas API keys in database Secure Canvas API token persistence with AES-GCM and legacy-safe read path May 29, 2026
Copilot AI requested a review from pconrad May 29, 2026 20:30
@pconrad

pconrad commented May 29, 2026

Copy link
Copy Markdown
Contributor

@copilot Can you add a short python script to the /docs/README_Canvas_API_Keys.md that will generate suitable values for CANVAS_API_TOKEN_ENCRYPTION_KEY?

Can you also add a database migration that clears all current values of the Canvas API Token field, and document that in the /docs/README_Canvas_API_Keys.md in a section on "Migration"

@pconrad

pconrad commented May 30, 2026

Copy link
Copy Markdown
Contributor

@copilot please look over the output of jacoco and pitest and add additional tests to cover the coverage gaps and surviving mutations

Copilot AI commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please look over the output of jacoco and pitest and add additional tests to cover the coverage gaps and surviving mutations

Addressed in eb5dd3c. I reviewed the PIT output for this branch and added focused tests in CanvasApiTokenSecurityServiceTests to cover the uncovered branches and kill the surviving mutations (null/empty paths, already-encrypted passthrough, malformed payload, random IV behavior, and invalid key formats/lengths). I also ran targeted tests plus focused PIT (targetClasses=...CanvasApiTokenSecurityService) and it now reports 100% mutation coverage for that class.

@pconrad

pconrad commented May 30, 2026

Copy link
Copy Markdown
Contributor

@copilot Can you add a database migration that sets all existing canvas tokens to null? The existing tokens will be in plaintext, so they should be removed and replaced with ones that are encrypted.

Copilot AI commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Can you add a database migration that sets all existing canvas tokens to null? The existing tokens will be in plaintext, so they should be removed and replaced with ones that are encrypted.

Implemented in 7aeddbc. I added a Liquibase migration (015-null-existing-canvas-api-tokens.json) that sets all existing COURSE.CANVAS_API_TOKEN values to NULL so legacy plaintext tokens are removed.

Copilot AI changed the title Secure Canvas API token persistence with AES-GCM and legacy-safe read path Secure Canvas API token persistence with AES-GCM and legacy token reset migration May 30, 2026
@pconrad

pconrad commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

@copilot Please run mvn git-code-format:format-code to fix the problems with 15-backend-format.

Then, please run mvn clean test jacoco:report, look at the report,and add tests to address the gaps in test coverage.

Copilot AI commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Please run mvn git-code-format:format-code to fix the problems with 15-backend-format.

Then, please run mvn clean test jacoco:report, look at the report,and add tests to address the gaps i...

Done in f6de7df. I ran mvn git-code-format:format-code, then mvn clean test jacoco:report, reviewed Jacoco, and added focused tests for uncovered CanvasApiTokenSecurityService exception paths. I reran mvn clean test jacoco:report; Canvas-related classes are now at 100% line coverage.

@pconrad

pconrad commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

@copilot There are still an uncovered branches in CanvasApiTokenSecurityService.java on lines 87 and 96.

Please write tests to be sure that all branches are covered on these lines.

Copilot AI commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

@copilot There are still an uncovered branches in CanvasApiTokenSecurityService.java on lines 87 and 96.

Please write tests to be sure that all branches are covered on these lines.

Implemented in faa0ff4. I added focused tests to cover the remaining branches on lines 87 and 96 in CanvasApiTokenSecurityService.java (null key path and valid 16-byte/24-byte key-length branches), then reran mvn clean test jacoco:report to verify those branch gaps are closed.

Comment thread docs/README_Canvas_API_Keys.md
@pconrad
pconrad marked this pull request as ready for review June 20, 2026 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Secure canvas api keys

2 participants