Skip to content
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.

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

SRE-474 - Add risk for each unsigned commit #11

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node-version: [12.x]
node-version: [14.20]
os: [ubuntu-latest]

steps:
Expand Down Expand Up @@ -41,7 +41,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node: [12]
node: [14.20]

steps:
- name: Initialize Output
Expand Down Expand Up @@ -69,7 +69,7 @@ jobs:
if: env.shouldpublishnpm
uses: actions/setup-node@v1
with:
node-version: 12.x
node-version: 14.20

- name: Build and Publish
id: build-and-publish
Expand Down
4 changes: 2 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
FROM node:14 as builder
FROM node:14.20.1 as builder
WORKDIR /opt/jupiterone
COPY . .
RUN yarn install && yarn build

FROM node:14-alpine
FROM node:14.20.1-alpine
Copy link

Choose a reason for hiding this comment

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

I thought we were moving away from alpine?

WORKDIR /opt/jupiterone
RUN apk update && apk upgrade && apk add --no-cache bash git gnupg
COPY --from=builder /opt/jupiterone/dist .
Expand Down
3 changes: 3 additions & 0 deletions defaultConfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@
"verifyGpg": {
"missingValue": 0.5
},
"verifyAllGpg": {
"perUnverifiedValue": 1
},
"gitleaksFindings": {
"perFindingValue": 10
}
Expand Down
9 changes: 8 additions & 1 deletion src/gather.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { Config, Risk, RiskCategory } from './types';

export async function localSCMRisk(): Promise<RiskCategory> {
const checks: Promise<Risk>[] = [];
const defaultRiskValue = 5;
const defaultRiskValue = 0;

const config = getConfig();

Expand All @@ -23,6 +23,12 @@ export async function localSCMRisk(): Promise<RiskCategory> {
}
if (config.facts.scm.gitPath && config.facts.scm.gpgPath) {
checks.push(scm.gpgVerifyRecentCommitsCheck(scmCheckValues.verifyGpg));
checks.push(
scm.gpgVerifyAllCommitsCheck(
config.flags.mergeRef,
scmCheckValues.verifyAllGpg
)
);
}

checks.push(
Expand All @@ -34,6 +40,7 @@ export async function localSCMRisk(): Promise<RiskCategory> {

// gather risks
const risks = await Promise.all(checks);
console.log({ risks, defaultRiskValue, section: 'localSCMRisk' });

return {
title: 'SCM Risk',
Expand Down
99 changes: 98 additions & 1 deletion src/scm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
SCMValuesEnforceGPGCheck,
SCMValuesGitCheck,
SCMValuesGitleaksFindingsCheck,
SCMValuesVerifyAllGPGCheck,
SCMValuesVerifyGPGCheck,
} from './types';

Expand All @@ -26,7 +27,7 @@ export async function gitRepoDirCheck(
let description = 'Missing - no repo found!';

if (await fs.pathExists(path.join(dir, '.git'))) {
value = -5;
value = -1;
description = 'Repo found. 🎉';
} else {
recommendations.push('Version code in a Git repository.');
Expand All @@ -44,6 +45,9 @@ export async function gitRepoDirCheck(
);
}

// TODO: remove this, or replace with a GitHub-aware check
// This doesn't work in CI/CD environments, and is not a good proxy for
// whether the repo is actually enforcing GPG signing.
export async function gitConfigGPGCheck(
checkValues: SCMValuesEnforceGPGCheck,
cmdRunner: any = undefined
Expand Down Expand Up @@ -75,6 +79,10 @@ export async function gitConfigGPGCheck(
);
}

// This is a best-effort verification of the top two commits (accounts for merge
// commits), and is proxy indicator that the PR is likely using signed
// commits. Does not unduly penalize for individual unsigned commits, but does
// reward in the presence of at least one recently signed commit.
export async function gpgVerifyRecentCommitsCheck(
checkValues: SCMValuesVerifyGPGCheck,
cmdRunner: any = undefined
Expand Down Expand Up @@ -107,6 +115,68 @@ export async function gpgVerifyRecentCommitsCheck(
);
}

// This is a thorough verification of signed commits for all commits in the PR.
// It will add risk for each unsigned, signed-but-expired, or signed-but-revoked
// commit.
export async function gpgVerifyAllCommitsCheck(
mergeRef: string,
checkValues: SCMValuesVerifyAllGPGCheck,
cmdRunner: any = undefined
): Promise<Risk> {
console.error({ mergeRef, checkValues });
const check = 'verifyGpgSigningAllCommits';
const recommendations: string[] = [];
const perUnverifiedValue = checkValues.perUnverifiedValue;

const validityLog = await gpgVerifyAllCommits(
`HEAD...${mergeRef}`,
cmdRunner
);
console.error({ validityLog });
// produces lines like:
// "G 3b8d0 John Doe commit message" # good signature
// "N 771cd John Doe commit message" # no signature
// "X ac731 John Smith commit message" # good signature that has expired
// "Y dead0 Jane Smith commit message" # good signature, key expired
// "R a001c John Doe commit message" # good signature, key revoked
// "U b10cc Jane Doe commit message" # good signature, key unknown
// "E 21a1c John Doe commit message" # signature cannot be checked, missing key

let description = `All commits in ${mergeRef} are signed.`;
let value = -1;

const unsignedCommits = validityLog
.split('\n')
.map((l) => l.split(' '))
.filter((e) => ['N', 'X', 'Y', 'R'].includes(e[0]));

if (unsignedCommits.length > 0) {
description = `Found ${unsignedCommits.length} unsigned commits since ${mergeRef}.`;
// add risk for each unsigned commit
value = perUnverifiedValue * unsignedCommits.length;
for (const splitLog of unsignedCommits) {
recommendations.push(
`Commit '${splitLog.join(
' '
)}' is unsigned or has an invalid signature.`
);
}
recommendations.push(
`Sign your commits with a verified GPG key. See https://git-scm.com/docs/pretty-formats#Documentation/pretty-formats.txt-emGem for more details on the concise git log '%G?' output.`
);
}

return formatRisk(
{
check,
value,
description,
recommendations,
},
riskCategory,
check
);
}
export async function gpgVerifyCommit(
gitref: string,
cmdRunner: any = undefined
Expand All @@ -115,6 +185,33 @@ export async function gpgVerifyCommit(
return Boolean(/NEWSIG/.exec(cmd.stderr)); // git puts this on stderr for some reason
}

export async function gpgVerifyAllCommits(
gitref: string,
cmdRunner: any = undefined
): Promise<string> {
// per https://git-scm.com/docs/pretty-formats#Documentation/pretty-formats.txt-emGem,
// produces lines like:
// "G 3b8d0 John Doe commit message" # good signature
// "X ac731 John Smith commit message" # good signature that has expired
// "Y dead0 Jane Smith commit message" # good signature, key expired
// "R a001c John Doe commit message" # good signature, key revoked
// "U b10cc Jane Doe commit message" # good signature, key unknown
// "E 21a1c John Doe commit message" # signature cannot be checked, missing key
// "N 771cd John Doe commit message" # no signature
//
// NOTE: All values in the first column of log output, other than 'N'
// rely on local GPG keys, so it is the users' responsibility to ensure
// that their local GPG keys are up to date and trusted if they want to
// rely on these values, prior to executing Peril.

const cmd = await runCmd(
`git log --pretty="format:%G? %h %aN %s" ${gitref} 2>&1`,
cmdRunner,
{ shell: true }
);
return cmd.stdout;
}

export async function gatherFacts(
cmdRunner: any = undefined,
config: Config = getConfig()
Expand Down
5 changes: 5 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export type SCMValues = {
git: SCMValuesGitCheck;
enforceGpg: SCMValuesEnforceGPGCheck;
verifyGpg: SCMValuesVerifyGPGCheck;
verifyAllGpg: SCMValuesVerifyAllGPGCheck;
gitleaksFindings: SCMValuesGitleaksFindingsCheck;
};
};
Expand All @@ -96,6 +97,10 @@ export type SCMValuesVerifyGPGCheck = {
missingValue: number;
};

export type SCMValuesVerifyAllGPGCheck = {
perUnverifiedValue: number;
};

export type SCMValuesGitleaksFindingsCheck = {
perFindingValue: number;
};
Expand Down