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

Add enable config parameter for legacy PNP signer endpoints #9967

Merged
merged 4 commits into from
Oct 21, 2022
Merged
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
1 change: 1 addition & 0 deletions .env
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ ODIS_SIGNER_DOCKER_IMAGE_TAG=oblivious-decentralized-identifier-service-1.1.10
ODIS_SIGNER_BLOCKCHAIN_PROVIDER=https://alfajores-forno.celo-testnet.org
ODIS_SIGNER_DOMAINS_API_ENABLED=true
ODIS_SIGNER_PNP_API_ENABLED=true
ODIS_SIGNER_LEGACY_PNP_API_ENABLED=true

# ODIS signer 0 Azure info
AZURE_ODIS0_CENTRALUS_AZURE_SUBSCRIPTION_ID=97e2b592-255b-4f92-bce0-127257163c36
Expand Down
3 changes: 1 addition & 2 deletions dependency-graph.json
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,7 @@
"@celo/identity",
"@celo/phone-number-privacy-common",
"@celo/utils",
"@celo/wallet-hsm-azure",
"@celo/wallet-rpc"
"@celo/wallet-hsm-azure"
]
},
"@celo/base": {
Expand Down
2 changes: 2 additions & 0 deletions packages/celotool/src/lib/env-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ export enum envVar {
ODIS_SIGNER_BLOCKCHAIN_PROVIDER = 'ODIS_SIGNER_BLOCKCHAIN_PROVIDER',
ODIS_SIGNER_DOMAINS_API_ENABLED = 'ODIS_SIGNER_DOMAINS_API_ENABLED',
ODIS_SIGNER_PNP_API_ENABLED = 'ODIS_SIGNER_PNP_API_ENABLED',
ODIS_SIGNER_LEGACY_PNP_API_ENABLED = 'ODIS_SIGNER_LEGACY_PNP_API_ENABLED',
ORACLE_DOCKER_IMAGE_REPOSITORY = 'ORACLE_DOCKER_IMAGE_REPOSITORY',
ORACLE_DOCKER_IMAGE_TAG = 'ORACLE_DOCKER_IMAGE_TAG',
ORACLE_UNUSED_ORACLE_ADDRESSES = 'ORACLE_UNUSED_ORACLE_ADDRESSES',
Expand Down Expand Up @@ -226,6 +227,7 @@ export enum DynamicEnvVar {
ODIS_SIGNER_AZURE_KEYVAULT_DOMAINS_KEY_LATEST_VERSION = '{{ context }}_ODIS_SIGNER_AZURE_KEYVAULT_DOMAINS_KEY_LATEST_VERSION',
ODIS_SIGNER_DOMAINS_API_ENABLED = '{{ context }}_ODIS_SIGNER_DOMAINS_API_ENABLED',
ODIS_SIGNER_PHONE_NUMBER_PRIVACY_API_ENABLED = '{{ context }}_ODIS_SIGNER_PNP_API_ENABLED',
ODIS_SIGNER_LEGACY_PHONE_NUMBER_PRIVACY_API_ENABLED = '{{ context }}_ODIS_SIGNER_LEGACY_PNP_API_ENABLED',
ODIS_SIGNER_DB_HOST = '{{ context }}_ODIS_SIGNER_DB_HOST',
ODIS_SIGNER_DB_PORT = '{{ context }}_ODIS_SIGNER_DB_PORT',
ODIS_SIGNER_DB_USERNAME = '{{ context }}_ODIS_SIGNER_DB_USERNAME',
Expand Down
1 change: 1 addition & 0 deletions packages/celotool/src/lib/odis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ async function helmParameters(celoEnv: string, context: string) {
`--set keystore.pnpKeyLatestVersion=${keyVaultConfig.pnpKeyLatestVersion}`,
`--set keystore.domainsKeyLatestVersion=${keyVaultConfig.domainsKeyLatestVersion}`,
`--set api.pnpAPIEnabled=${fetchEnv(envVar.ODIS_SIGNER_PNP_API_ENABLED)}`,
`--set api.legacyPnpAPIEnabled=${fetchEnv(envVar.ODIS_SIGNER_LEGACY_PNP_API_ENABLED)}`,
`--set api.domainsAPIEnabled=${fetchEnv(envVar.ODIS_SIGNER_DOMAINS_API_ENABLED)}`,
`--set blockchainProvider=${fetchEnv(envVar.ODIS_SIGNER_BLOCKCHAIN_PROVIDER)}`,
`--set blockchainApiKey=${blockchainConfig.blockchainApiKey}`,
Expand Down
1 change: 1 addition & 0 deletions packages/helm-charts/odis/templates/signer-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ spec:
{{ include "common.env-var" (dict "name" "DOMAINS_LATEST_KEY_VERSION" "dict" .Values.keystore "value_name" "domainsKeyLatestVersion") | indent 12 }}
{{ include "common.env-var" (dict "name" "DOMAINS_API_ENABLED" "dict" .Values.api "value_name" "domainsAPIEnabled") | indent 12 }}
{{ include "common.env-var" (dict "name" "PHONE_NUMBER_PRIVACY_API_ENABLED" "dict" .Values.api "value_name" "pnpAPIEnabled") | indent 12 }}
{{ include "common.env-var" (dict "name" "LEGACY_PHONE_NUMBER_PRIVACY_API_ENABLED" "dict" .Values.api "value_name" "legacyPnpAPIEnabled") | indent 12 }}
- name: DB_PASSWORD
valueFrom:
secretKeyRef:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ const signerConfig: SignerConfig = {
enabled: false,
shouldFailOpen: true,
},
legacyPhoneNumberPrivacy: {
enabled: false,
shouldFailOpen: true,
},
},
attestations: {
numberAttestationsRequired: 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ const signerConfig: SignerConfig = {
enabled: false,
},
phoneNumberPrivacy: {
enabled: false,
shouldFailOpen: true,
},
legacyPhoneNumberPrivacy: {
enabled: true,
shouldFailOpen: true,
},
Expand Down Expand Up @@ -729,7 +733,7 @@ describe(`legacyPnpService: ${CombinerEndpoint.LEGACY_PNP_SIGN}`, () => {
describe('when 2/3 of signers are disabled', () => {
beforeEach(async () => {
const configWithApiDisabled: SignerConfig = JSON.parse(JSON.stringify(signerConfig))
configWithApiDisabled.api.phoneNumberPrivacy.enabled = false
configWithApiDisabled.api.legacyPhoneNumberPrivacy.enabled = false
signer1 = startSigner(signerConfig, signerDB1, keyProvider1).listen(3001)
signer2 = startSigner(configWithApiDisabled, signerDB2, keyProvider2).listen(3002)
signer3 = startSigner(configWithApiDisabled, signerDB3, keyProvider3).listen(3003)
Expand All @@ -750,7 +754,7 @@ describe(`legacyPnpService: ${CombinerEndpoint.LEGACY_PNP_SIGN}`, () => {
describe('when 1/3 of signers are disabled', () => {
beforeEach(async () => {
const configWithApiDisabled: SignerConfig = JSON.parse(JSON.stringify(signerConfig))
configWithApiDisabled.api.phoneNumberPrivacy.enabled = false
configWithApiDisabled.api.legacyPhoneNumberPrivacy.enabled = false
signer1 = startSigner(signerConfig, signerDB1, keyProvider1).listen(3001)
signer2 = startSigner(signerConfig, signerDB2, keyProvider2).listen(3002)
signer3 = startSigner(configWithApiDisabled, signerDB3, keyProvider3).listen(3003)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ const signerConfig: SignerConfig = {
enabled: true,
shouldFailOpen: true,
},
legacyPhoneNumberPrivacy: {
enabled: false,
shouldFailOpen: true,
},
},
attestations: {
numberAttestationsRequired: 3,
Expand Down
2 changes: 0 additions & 2 deletions packages/phone-number-privacy/common/src/test/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ import {
import { signWithRawKey } from '../utils/authentication'
import { genSessionID } from '../utils/logger'

jest.fn<AttestationsStatus, []>()

export function createMockAttestation(getVerifiedStatus: jest.Mock<AttestationsStatus, []>) {
return {
getVerifiedStatus,
Expand Down
1 change: 0 additions & 1 deletion packages/phone-number-privacy/signer/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
"promise.allsettled": "^1.0.2"
},
"devDependencies": {
"@celo/wallet-rpc": "2.2.1-dev",
"@types/btoa": "^1.2.3",
"@types/express": "^4.17.6",
"@types/supertest": "^2.0.12",
Expand Down
8 changes: 8 additions & 0 deletions packages/phone-number-privacy/signer/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ export interface SignerConfig {
enabled: boolean
shouldFailOpen: boolean
}
legacyPhoneNumberPrivacy: {
enabled: boolean
shouldFailOpen: boolean
}
}
attestations: {
numberAttestationsRequired: number
Expand Down Expand Up @@ -125,6 +129,10 @@ export const config: SignerConfig = {
enabled: toBool(env.PHONE_NUMBER_PRIVACY_API_ENABLED, false),
shouldFailOpen: toBool(env.FULL_NODE_ERRORS_SHOULD_FAIL_OPEN, true),
},
legacyPhoneNumberPrivacy: {
enabled: toBool(env.LEGACY_PHONE_NUMBER_PRIVACY_API_ENABLED, false),
shouldFailOpen: toBool(env.LEGACY_FULL_NODE_ERRORS_SHOULD_FAIL_OPEN, true),
},
},
attestations: {
numberAttestationsRequired: Number(env.ATTESTATIONS_NUMBER_ATTESTATIONS_REQUIRED ?? 3),
Expand Down
8 changes: 4 additions & 4 deletions packages/phone-number-privacy/signer/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ export function startSigner(
legacyPnpQuotaService,
keyProvider,
new LegacyPnpSignIO(
config.api.phoneNumberPrivacy.enabled,
config.api.phoneNumberPrivacy.shouldFailOpen,
config.api.legacyPhoneNumberPrivacy.enabled,
config.api.legacyPhoneNumberPrivacy.shouldFailOpen,
kit
)
)
Expand All @@ -133,8 +133,8 @@ export function startSigner(
config,
legacyPnpQuotaService,
new LegacyPnpQuotaIO(
config.api.phoneNumberPrivacy.enabled,
config.api.phoneNumberPrivacy.shouldFailOpen,
config.api.legacyPhoneNumberPrivacy.enabled,
config.api.legacyPhoneNumberPrivacy.shouldFailOpen,
kit
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import {
SequentialDelayDomain,
SignerEndpoint as Endpoint,
} from '@celo/phone-number-privacy-common'
import { ACCOUNT_ADDRESS2 } from '@celo/phone-number-privacy-common/lib/test/values'
import { defined, noBool, noString } from '@celo/utils/lib/sign-typed-data-utils'
import { ACCOUNT_ADDRESS2 } from '@celo/wallet-rpc/lib/rpc-wallet.test'
import 'isomorphic-fetch'
import { contractKit } from '../../../../combiner/test/end-to-end/resources'

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe('legacyPNP', () => {
const _config: typeof config = JSON.parse(JSON.stringify(config))
_config.db.type = SupportedDatabase.Sqlite
_config.keystore.type = SupportedKeystore.MOCK_SECRET_MANAGER
_config.api.phoneNumberPrivacy.enabled = true
_config.api.legacyPhoneNumberPrivacy.enabled = true

const expectedSignature = expectedSignatures[_config.keystore.keys.phoneNumberPrivacy.latest - 1]

Expand Down Expand Up @@ -569,7 +569,7 @@ describe('legacyPNP', () => {

it('Should respond with 503 on disabled api', async () => {
const configWithApiDisabled: typeof _config = JSON.parse(JSON.stringify(_config))
configWithApiDisabled.api.phoneNumberPrivacy.enabled = false
configWithApiDisabled.api.legacyPhoneNumberPrivacy.enabled = false
const appWithApiDisabled = startSigner(configWithApiDisabled, db, keyProvider)
const req = getLegacyPnpQuotaRequest(ACCOUNT_ADDRESS1, IDENTIFIER)
const authorization = getPnpRequestAuthorization(req, PRIVATE_KEY1)
Expand Down Expand Up @@ -1148,7 +1148,7 @@ describe('legacyPNP', () => {

it('Should respond with 503 on disabled api', async () => {
const configWithApiDisabled: typeof _config = JSON.parse(JSON.stringify(_config))
configWithApiDisabled.api.phoneNumberPrivacy.enabled = false
configWithApiDisabled.api.legacyPhoneNumberPrivacy.enabled = false
const appWithApiDisabled = startSigner(configWithApiDisabled, db, keyProvider)

const req = getLegacyPnpSignRequest(
Expand All @@ -1174,6 +1174,14 @@ describe('legacyPNP', () => {
})

describe('interactions between legacy and new endpoints', () => {
const configWithPNPEnabled: typeof _config = JSON.parse(JSON.stringify(_config))
configWithPNPEnabled.api.phoneNumberPrivacy.enabled = true
let appWithPNPEnabled: any

beforeEach(() => {
appWithPNPEnabled = startSigner(configWithPNPEnabled, db, keyProvider)
})

// Keep both of these cases with the legacy test suite
// since once this endpoint is deprecated, these tests will no longer be needed
it('Should not be affected by requests and queries from the new endpoint', async () => {
Expand All @@ -1185,7 +1193,13 @@ describe('legacyPNP', () => {
AuthenticationMethod.WALLET_KEY
)
const authorization = getPnpRequestAuthorization(req, PRIVATE_KEY1)
const res = await sendRequest(req, authorization, SignerEndpoint.PNP_SIGN)
const res = await sendRequest(
req,
authorization,
SignerEndpoint.PNP_SIGN,
undefined,
appWithPNPEnabled
)
expect(res.status).toBe(200)
expect(res.body).toStrictEqual<SignMessageResponseSuccess>({
success: true,
Expand All @@ -1209,7 +1223,9 @@ describe('legacyPNP', () => {
const legacyRes = await sendRequest(
legacyReq,
legacyAuthorization,
SignerEndpoint.LEGACY_PNP_SIGN
SignerEndpoint.LEGACY_PNP_SIGN,
undefined,
appWithPNPEnabled
)
expect(legacyRes.status).toBe(200)
expect(legacyRes.body).toStrictEqual<SignMessageResponseSuccess>({
Expand All @@ -1226,7 +1242,7 @@ describe('legacyPNP', () => {
)
})

it('Should affect the requests and queries to the new endpoint', async () => {
it('Should not affect the requests and queries to the new endpoint', async () => {
const legacyReq = getLegacyPnpSignRequest(
ACCOUNT_ADDRESS1,
BLINDED_PHONE_NUMBER,
Expand All @@ -1237,7 +1253,9 @@ describe('legacyPNP', () => {
const legacyRes = await sendRequest(
legacyReq,
legacyAuthorization,
SignerEndpoint.LEGACY_PNP_SIGN
SignerEndpoint.LEGACY_PNP_SIGN,
undefined,
appWithPNPEnabled
)
expect(legacyRes.status).toBe(200)
expect(legacyRes.body).toStrictEqual<SignMessageResponseSuccess>({
Expand All @@ -1260,7 +1278,13 @@ describe('legacyPNP', () => {
AuthenticationMethod.WALLET_KEY
)
const authorization = getPnpRequestAuthorization(req, PRIVATE_KEY1)
const res = await sendRequest(req, authorization, SignerEndpoint.PNP_SIGN)
const res = await sendRequest(
req,
authorization,
SignerEndpoint.PNP_SIGN,
undefined,
appWithPNPEnabled
)
expect(res.status).toBe(200)
expect(res.body).toStrictEqual<SignMessageResponseSuccess>({
success: true,
Expand Down