Skip to content

Commit

Permalink
Add enable config parameter for legacy PNP signer endpoints (#9967)
Browse files Browse the repository at this point in the history
* Add separate enable config value for legacy PNP signer endpoint

* Driveby change: remove unnecessary jest line from test utils

* Add legacy enabled param to celotool and helm charts

* Remove rpc-wallet dep
  • Loading branch information
eelanagaraj committed Oct 21, 2022
1 parent 15f823f commit e9601d4
Show file tree
Hide file tree
Showing 14 changed files with 65 additions and 20 deletions.
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

0 comments on commit e9601d4

Please sign in to comment.