Skip to content

feat(pqc): add post quantum cryptography safe showcase integration tests#8875

Draft
danieljbruce wants to merge 2 commits into
mainfrom
feat-pqc-showcase-tests-11540590886686220413
Draft

feat(pqc): add post quantum cryptography safe showcase integration tests#8875
danieljbruce wants to merge 2 commits into
mainfrom
feat-pqc-showcase-tests-11540590886686220413

Conversation

@danieljbruce

Copy link
Copy Markdown
Contributor

This submission implements post-quantumsafe integration tests for both gRPC and HTTP/REST Fallback connections to a TLS-enabled gapic-showcase server (running v0.41.1). The tests assert that the X25519MLKEM768 hybrid key exchange group is successfully negotiated.


PR created automatically by Jules for task 11540590886686220413 started by @danieljbruce

Adds gRPC and HTTP/REST Fallback post quantum cryptography safe integration tests to the test-application package.
Configures ShowcaseServer to support TLS, specific ports, and CA certificate output files.
Asserts that X25519MLKEM768 is correctly negotiated.

Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for TLS, custom ports, and CA certificate output files in the ShowcaseServer, and adds integration tests for Post Quantum Cryptography (PQC) over both gRPC and HTTP/REST Fallback. The review feedback highlights potential TypeScript compilation errors under strictNullChecks when using this.originalCwd directly, a directory restoration failure if start() throws an error before this.server is initialized, and a potential runtime TypeError in the overridden fetch method if opts is not provided.

Comment on lines +36 to +38
async start(opts?: { tls?: boolean; port?: string; caCertOutputFile?: string }) {
this.originalCwd = process.cwd();
const testDir = path.join(this.originalCwd, '.showcase-server-dir');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are two issues here:

  1. TypeScript Strict Null Checks: Since this.originalCwd is typed as string | undefined, passing it directly to path.join and path.resolve will cause compilation errors under strictNullChecks. Using a local cwd variable avoids this.
  2. Error Masking & Directory Restoration Failure: If start() fails (e.g., due to download or port issues) before this.server is assigned, this.server remains undefined. When stop() is called in the finally block, it will throw an error because this.server is missing, which masks the original failure and prevents process.chdir from restoring the directory. Initializing this.server with a dummy object ensures stop() can safely restore the directory and won't mask the real error.
Suggested change
async start(opts?: { tls?: boolean; port?: string; caCertOutputFile?: string }) {
this.originalCwd = process.cwd();
const testDir = path.join(this.originalCwd, '.showcase-server-dir');
async start(opts?: { tls?: boolean; port?: string; caCertOutputFile?: string }) {
const cwd = process.cwd();
this.originalCwd = cwd;
this.server = { kill: () => {} } as any;
const testDir = path.join(cwd, '.showcase-server-dir');

Comment on lines +46 to +49
let resolvedCaCertPath = '';
if (opts?.caCertOutputFile) {
resolvedCaCertPath = path.resolve(this.originalCwd, opts.caCertOutputFile);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the local cwd variable instead of this.originalCwd to avoid TypeScript compilation errors under strictNullChecks.

Suggested change
let resolvedCaCertPath = '';
if (opts?.caCertOutputFile) {
resolvedCaCertPath = path.resolve(this.originalCwd, opts.caCertOutputFile);
}
let resolvedCaCertPath = '';
if (opts?.caCertOutputFile) {
resolvedCaCertPath = path.resolve(cwd, opts.caCertOutputFile);
}

Comment on lines +2979 to +2980
(auth as any).fetch = async (url: string, opts: any) => {
if (url.startsWith('https:')) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential runtime TypeError crashes if opts is not provided (since opts is optional in the fetch signature), initialize opts to an empty object if it is falsy before accessing or setting its properties.

  (auth as any).fetch = async (url: string, opts: any) => {
    opts = opts || {};
    if (url.startsWith('https:')) {

Adds gRPC and HTTP/REST Fallback post quantum cryptography safe integration tests to the test-application package.
Configures ShowcaseServer to support TLS, specific ports, and CA certificate output files.
Asserts that X25519MLKEM768 is correctly negotiated.

Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com>
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.

1 participant