-
Notifications
You must be signed in to change notification settings - Fork 2
Ed25519 crypto infrastructure #165
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
teddsa_core is duplicate. I thinkg it should be moved into frost_ed25519_keplr
| }); | ||
| router.post( | ||
| "/keygen_ed25519", | ||
| oauthMiddleware, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, this should be fixed by adding an Express rate-limiting middleware to the route so that expensive, authorization-protected operations like key generation cannot be triggered arbitrarily often by a single client. A standard way in an Express app is to use the express-rate-limit package (or a similar well-known library) and apply a limiter either globally or per-route, keyed by IP or user identity.
For this specific route, the least invasive fix is to add a per-route limiter using express-rate-limit. Because we can only modify the provided snippet, we will: (1) import express-rate-limit at the top of backend/tss_api/src/routes/keygen_ed25519.ts; (2) define a keygenEd25519Limiter constant after the imports, configured to allow a reasonable number of requests within a time window (e.g., 10 requests per minute, or another conservative value; we’ll pick a modest value without changing existing behavior otherwise); and (3) insert this limiter into the router.post middleware chain before oauthMiddleware, so that abusive traffic is throttled before hitting authentication and the expensive runKeygenEd25519 operation. No changes are needed to the route handler logic itself.
Concretely:
-
Add
import rateLimit from "express-rate-limit";near the other imports. -
Add a new constant, e.g.,
const keygenEd25519Limiter = rateLimit({ windowMs: 60 * 1000, // 1 minute max: 10, standardHeaders: true, legacyHeaders: false, });
-
Modify the
router.post("/keygen_ed25519", ...)call to includekeygenEd25519Limiteras the first middleware argument:router.post("/keygen_ed25519", keygenEd25519Limiter, oauthMiddleware, tssActivateMiddleware, async (...) => { ... }).
This addresses all the alert variants by adding rate limiting to the route that performs authorization.
-
Copy modified line R24 -
Copy modified lines R26-R32 -
Copy modified line R90
| @@ -21,7 +21,15 @@ | ||
| } from "@oko-wallet-tss-api/middleware/oauth"; | ||
| import { tssActivateMiddleware } from "@oko-wallet-tss-api/middleware/tss_activate"; | ||
| import type { OAuthLocals } from "@oko-wallet-tss-api/middleware/types"; | ||
| import rateLimit from "express-rate-limit"; | ||
|
|
||
| const keygenEd25519Limiter = rateLimit({ | ||
| windowMs: 60 * 1000, // 1 minute | ||
| max: 10, // limit each IP to 10 requests per windowMs | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
|
|
||
| export function setKeygenEd25519Routes(router: Router) { | ||
| registry.registerPath({ | ||
| method: "post", | ||
| @@ -80,6 +87,7 @@ | ||
| }); | ||
| router.post( | ||
| "/keygen_ed25519", | ||
| keygenEd25519Limiter, | ||
| oauthMiddleware, | ||
| tssActivateMiddleware, | ||
| async ( |
-
Copy modified lines R36-R37
| @@ -33,7 +33,8 @@ | ||
| "pg": "^8.16.3", | ||
| "swagger-ui-express": "^5.0.1", | ||
| "uuid": "^13.0.0", | ||
| "zod": "^4.1.12" | ||
| "zod": "^4.1.12", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/cors": "^2.8.17", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
|
|
||
| router.post( | ||
| "/presign_ed25519", | ||
| [apiKeyMiddleware, userJwtMiddleware, tssActivateMiddleware], |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, the fix is to add a rate-limiting middleware to the route so that expensive operations (DB and cryptographic work in runPresignEd25519) cannot be triggered arbitrarily often by an attacker. In Express-like apps, this is commonly done via a well-known library such as express-rate-limit, configured with a sensible per-IP or per-API-key quota and a time window.
For this specific route, the least intrusive change is to: (1) import express-rate-limit in this file, (2) define a limiter instance (e.g., presignEd25519Limiter) with a reasonable window and max value, and (3) include that limiter in the middleware array for the router.post("/presign_ed25519", ...) definition, ahead of or alongside the existing auth middlewares. This keeps current functionality intact—same auth checks and business logic—while simply rejecting clients who exceed the configured rate. Concretely, in backend/tss_api/src/routes/presign_ed25519.ts, we will add an import for express-rate-limit, define a limiter constant right after the imports, and update line 87’s middleware array to include the limiter.
-
Copy modified line R22 -
Copy modified lines R24-R28 -
Copy modified line R92
| @@ -19,7 +19,13 @@ | ||
| } from "@oko-wallet-tss-api/middleware/keplr_auth"; | ||
| import { apiKeyMiddleware } from "@oko-wallet-tss-api/middleware/api_key_auth"; | ||
| import { tssActivateMiddleware } from "@oko-wallet-tss-api/middleware/tss_activate"; | ||
| import rateLimit from "express-rate-limit"; | ||
|
|
||
| const presignEd25519Limiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs for this endpoint | ||
| }); | ||
|
|
||
| export function setPresignEd25519Routes(router: Router) { | ||
| registry.registerPath({ | ||
| method: "post", | ||
| @@ -84,7 +89,7 @@ | ||
|
|
||
| router.post( | ||
| "/presign_ed25519", | ||
| [apiKeyMiddleware, userJwtMiddleware, tssActivateMiddleware], | ||
| [presignEd25519Limiter, apiKeyMiddleware, userJwtMiddleware, tssActivateMiddleware], | ||
| async ( | ||
| req: UserAuthenticatedRequest<PresignEd25519Body>, | ||
| res: Response<OkoApiResponse<PresignEd25519Response>>, |
-
Copy modified lines R36-R37
| @@ -33,7 +33,8 @@ | ||
| "pg": "^8.16.3", | ||
| "swagger-ui-express": "^5.0.1", | ||
| "uuid": "^13.0.0", | ||
| "zod": "^4.1.12" | ||
| "zod": "^4.1.12", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/cors": "^2.8.17", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
|
|
||
| router.post( | ||
| "/sign_ed25519/round1", | ||
| [apiKeyMiddleware, userJwtMiddleware, tssActivateMiddleware], |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, the fix is to introduce a rate-limiting middleware (for example using express-rate-limit) and apply it to the sensitive route so that each client can only hit the endpoint a bounded number of times per time window. This minimizes the risk that an attacker can exhaust server resources by making many requests.
For this specific file, the least invasive fix is to:
- Import a well-known rate-limiting library such as
express-rate-limit. - Define a limiter instance configured appropriately for TSS signing endpoints (e.g., smallish number of requests per minute per IP or per key).
- Add that limiter to the middleware array for the
/sign_ed25519/round1route, before the handler. This preserves existing behavior while just adding throttling.
Concretely:
- At the top of
backend/tss_api/src/routes/sign_ed25519.ts, add an import forexpress-rate-limit. - Inside
setSignEd25519Routes, define a constant likesignEd25519Limiter = rateLimit({ windowMs: ..., max: ... }). - Update the
router.post("/sign_ed25519/round1", [...], async ...)call to includesignEd25519Limiterin its middleware array, e.g.[apiKeyMiddleware, userJwtMiddleware, tssActivateMiddleware, signEd25519Limiter]or earlier in the chain depending on desired behavior. This only affects the rate at which requests are allowed, without changing core business logic or responses.
-
Copy modified line R2 -
Copy modified lines R36-R41 -
Copy modified line R105
| @@ -1,4 +1,5 @@ | ||
| import type { Response, Router } from "express"; | ||
| import rateLimit from "express-rate-limit"; | ||
| import type { | ||
| SignEd25519Round1Body, | ||
| SignEd25519Round1Response, | ||
| @@ -32,6 +33,12 @@ | ||
| import { tssActivateMiddleware } from "@oko-wallet-tss-api/middleware/tss_activate"; | ||
|
|
||
| export function setSignEd25519Routes(router: Router) { | ||
| const signEd25519Limiter = rateLimit({ | ||
| windowMs: 60 * 1000, // 1 minute | ||
| max: 10, // limit each IP to 10 requests per windowMs for signing operations | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
| registry.registerPath({ | ||
| method: "post", | ||
| path: "/tss/v1/sign_ed25519/round1", | ||
| @@ -95,7 +102,7 @@ | ||
|
|
||
| router.post( | ||
| "/sign_ed25519/round1", | ||
| [apiKeyMiddleware, userJwtMiddleware, tssActivateMiddleware], | ||
| [apiKeyMiddleware, userJwtMiddleware, tssActivateMiddleware, signEd25519Limiter], | ||
| async ( | ||
| req: UserAuthenticatedRequest<SignEd25519Round1Body>, | ||
| res: Response<OkoApiResponse<SignEd25519Round1Response>>, |
-
Copy modified lines R36-R37
| @@ -33,7 +33,8 @@ | ||
| "pg": "^8.16.3", | ||
| "swagger-ui-express": "^5.0.1", | ||
| "uuid": "^13.0.0", | ||
| "zod": "^4.1.12" | ||
| "zod": "^4.1.12", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/cors": "^2.8.17", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
|
|
||
| router.post( | ||
| "/sign_ed25519/round2", | ||
| [userJwtMiddleware, tssActivateMiddleware], |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, to fix missing rate limiting on an Express route, you introduce a rate-limiting middleware (commonly from express-rate-limit), configure an appropriate window and request cap, and attach it to the affected routes (or router) in the middleware chain. This ensures that even authenticated users cannot exceed a safe request rate, mitigating DoS risks from expensive operations like cryptographic signing or database-heavy workflows.
For this specific file, the best minimal-impact fix is:
- Import
express-rate-limit. - Define a rate limiter specifically for Ed25519 signing operations (e.g.,
signEd25519RateLimiter) with reasonable defaults (e.g.,windowMsof a few minutes and a limited number of requests per window per IP). We will keep the config simple and self-contained in this file. - Apply this rate limiter to at least the
/sign_ed25519/round2route highlighted by CodeQL by adding it to the middleware array, before the existing auth middlewares or between them, depending on preference. To keep semantics straightforward and avoid changing auth behavior, we will insert the limiter at the beginning of the middleware array so that rate limiting occurs before deeper work. - Optionally, and still within this file, we could also reuse this limiter for
/sign_ed25519/round1and any other expensive signing routes for consistent protection. However, to stay strictly focused on the reported issue, we will only change theround2route.
Concretely:
- In
backend/tss_api/src/routes/sign_ed25519.ts, add an import forexpress-rate-limit. - Right after the imports and before
export function setSignEd25519Routes, defineconst signEd25519RateLimiter = rateLimit({ ... }). - Update the
router.post("/sign_ed25519/round2", [userJwtMiddleware, tssActivateMiddleware], ...)call so that the middleware array becomes[signEd25519RateLimiter, userJwtMiddleware, tssActivateMiddleware].
-
Copy modified line R20 -
Copy modified lines R35-R41 -
Copy modified line R202
| @@ -17,6 +17,7 @@ | ||
| } from "@oko-wallet/oko-api-openapi/common"; | ||
| import { registry } from "@oko-wallet/oko-api-openapi"; | ||
|
|
||
| import rateLimit from "express-rate-limit"; | ||
| import { | ||
| runSignEd25519Round1, | ||
| runSignEd25519Round2, | ||
| @@ -31,6 +32,13 @@ | ||
| import { apiKeyMiddleware } from "@oko-wallet-tss-api/middleware/api_key_auth"; | ||
| import { tssActivateMiddleware } from "@oko-wallet-tss-api/middleware/tss_activate"; | ||
|
|
||
| const signEd25519RateLimiter = rateLimit({ | ||
| windowMs: 1 * 60 * 1000, // 1 minute window | ||
| max: 30, // limit each IP to 30 requests per windowMs for signing operations | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
|
|
||
| export function setSignEd25519Routes(router: Router) { | ||
| registry.registerPath({ | ||
| method: "post", | ||
| @@ -191,7 +199,7 @@ | ||
|
|
||
| router.post( | ||
| "/sign_ed25519/round2", | ||
| [userJwtMiddleware, tssActivateMiddleware], | ||
| [signEd25519RateLimiter, userJwtMiddleware, tssActivateMiddleware], | ||
| async ( | ||
| req: UserAuthenticatedRequest<SignEd25519Round2Body>, | ||
| res: Response<OkoApiResponse<SignEd25519Round2Response>>, |
-
Copy modified lines R36-R37
| @@ -33,7 +33,8 @@ | ||
| "pg": "^8.16.3", | ||
| "swagger-ui-express": "^5.0.1", | ||
| "uuid": "^13.0.0", | ||
| "zod": "^4.1.12" | ||
| "zod": "^4.1.12", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/cors": "^2.8.17", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
|
|
||
| router.post( | ||
| "/sign_ed25519", | ||
| [userJwtMiddleware, tssActivateMiddleware], |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
|
|
||
| router.post( | ||
| "/sign_ed25519/aggregate", | ||
| [userJwtMiddleware, tssActivateMiddleware], |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, the fix is to introduce rate limiting for this route, using a standard middleware such as express-rate-limit, and apply it alongside the existing authentication and activation middlewares. This ensures that even authenticated users cannot overwhelm the service with excessive /sign_ed25519/aggregate requests.
The best way to fix this code without changing functionality is:
- Import a rate-limiting middleware (e.g.,
express-rate-limit) inbackend/tss_api/src/routes/sign_ed25519.ts. - Define a limiter specifically for TSS signing routes (or reuse one defined in this file) with sensible defaults, such as a small number of requests per short time window, since signing is likely expensive.
- Insert that limiter into the middleware array of the
/sign_ed25519/aggregateroute, keeping the order so that authentication still runs before the handler but allowing the limiter to run after auth (or before, as desired) and before the heavy operationrunSignEd25519Aggregate.
Concretely in this file:
- Add an import for
express-rate-limit. - Add a
const signEd25519RateLimiter = rateLimit({...})definition near the top ofsetSignEd25519Routesor module-level so it is reused and initialized once. - Update the
router.post("/sign_ed25519/aggregate", ...)call so its middleware array becomes[userJwtMiddleware, tssActivateMiddleware, signEd25519RateLimiter](or similar).
-
Copy modified line R19 -
Copy modified lines R35-R41 -
Copy modified line R413
| @@ -16,6 +16,7 @@ | ||
| UserAuthHeaderSchema, | ||
| } from "@oko-wallet/oko-api-openapi/common"; | ||
| import { registry } from "@oko-wallet/oko-api-openapi"; | ||
| import rateLimit from "express-rate-limit"; | ||
|
|
||
| import { | ||
| runSignEd25519Round1, | ||
| @@ -31,6 +32,13 @@ | ||
| import { apiKeyMiddleware } from "@oko-wallet-tss-api/middleware/api_key_auth"; | ||
| import { tssActivateMiddleware } from "@oko-wallet-tss-api/middleware/tss_activate"; | ||
|
|
||
| const signEd25519RateLimiter = rateLimit({ | ||
| windowMs: 60 * 1000, // 1 minute | ||
| max: 30, // limit each IP/user to 30 aggregate/sign requests per minute | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
|
|
||
| export function setSignEd25519Routes(router: Router) { | ||
| registry.registerPath({ | ||
| method: "post", | ||
| @@ -402,7 +410,7 @@ | ||
|
|
||
| router.post( | ||
| "/sign_ed25519/aggregate", | ||
| [userJwtMiddleware, tssActivateMiddleware], | ||
| [userJwtMiddleware, tssActivateMiddleware, signEd25519RateLimiter], | ||
| async ( | ||
| req: UserAuthenticatedRequest<SignEd25519AggregateBody>, | ||
| res: Response<OkoApiResponse<SignEd25519AggregateResponse>>, |
-
Copy modified lines R36-R37
| @@ -33,7 +33,8 @@ | ||
| "pg": "^8.16.3", | ||
| "swagger-ui-express": "^5.0.1", | ||
| "uuid": "^13.0.0", | ||
| "zod": "^4.1.12" | ||
| "zod": "^4.1.12", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/cors": "^2.8.17", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
|
|
||
| router.post( | ||
| "/wallet_ed25519/public_info", | ||
| oauthMiddleware, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
This route handler performs
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, this kind of problem is fixed by adding a dedicated rate-limiting middleware to the route (or to a router that it uses), so that requests hitting expensive, authorization-protected operations are throttled. In Express, this is commonly done using a well-known library such as express-rate-limit. The middleware should be applied in the route’s middleware chain, ideally after basic authentication/authorization (so limits can be per-user if desired) but before the expensive handler logic.
For this specific route in backend/tss_api/src/routes/wallet_ed25519.ts, the minimal, non-breaking change is:
- Import
express-rate-limit. - Define a limiter configured for this route (e.g., allow a conservative but reasonable number of POSTs per time window).
- Insert that limiter into the
router.postmiddleware list for/wallet_ed25519/public_info, alongsideoauthMiddlewareandtssActivateMiddleware.
Because we cannot see or modify other files, the limiter must be defined in this file. A sensible configuration that doesn’t change functional behavior except under abusive load might be, for example, 30 requests per minute per IP. We’ll:
- Add
import rateLimit from "express-rate-limit";near the existing imports. - Create a
walletEd25519PublicInfoLimiterconstant usingrateLimit({ windowMs: 60_000, max: 30, standardHeaders: true, legacyHeaders: false }). - Update the
router.postcall so that the middleware chain isoauthMiddleware, walletEd25519PublicInfoLimiter, tssActivateMiddleware, async (...) => { ... }. Placing rate limiting afteroauthMiddlewareallows you to later choose a key function based on user identity if desired, but still limits abusive traffic early in the chain.
-
Copy modified line R9 -
Copy modified lines R22-R28 -
Copy modified line R85
| @@ -6,6 +6,7 @@ | ||
| OAuthHeaderSchema, | ||
| } from "@oko-wallet/oko-api-openapi/common"; | ||
| import { registry } from "@oko-wallet/oko-api-openapi"; | ||
| import rateLimit from "express-rate-limit"; | ||
|
|
||
| import { | ||
| getWalletEd25519PublicInfo, | ||
| @@ -18,6 +19,13 @@ | ||
| import type { OAuthLocals } from "@oko-wallet-tss-api/middleware/types"; | ||
| import { tssActivateMiddleware } from "@oko-wallet-tss-api/middleware/tss_activate"; | ||
|
|
||
| const walletEd25519PublicInfoLimiter = rateLimit({ | ||
| windowMs: 60 * 1000, // 1 minute window | ||
| max: 30, // limit each IP to 30 requests per windowMs | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
|
|
||
| export function setWalletEd25519Routes(router: Router) { | ||
| registry.registerPath({ | ||
| method: "post", | ||
| @@ -74,6 +82,7 @@ | ||
| router.post( | ||
| "/wallet_ed25519/public_info", | ||
| oauthMiddleware, | ||
| walletEd25519PublicInfoLimiter, | ||
| tssActivateMiddleware, | ||
| async ( | ||
| req: OAuthAuthenticatedRequest<Record<string, never>>, |
-
Copy modified lines R36-R37
| @@ -33,7 +33,8 @@ | ||
| "pg": "^8.16.3", | ||
| "swagger-ui-express": "^5.0.1", | ||
| "uuid": "^13.0.0", | ||
| "zod": "^4.1.12" | ||
| "zod": "^4.1.12", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/cors": "^2.8.17", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
| const payload: UserTokenPayload = { | ||
| email: args.email, | ||
| wallet_id: args.wallet_id, | ||
| wallet_id_ed25519: args.wallet_id_ed25519, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate wallet_id_ed25519 was required because each curve type is stored as a distinct wallet record in the database. Do you have a better suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on a coding style. I think it would be better that wallet_id is replaced with object like this.
wallet_ids: {
[curveType: "secp256k1" | "ed25519"]: string
}
|
Something might cause boundary errors about point_x. I will figure it out & fix it. |
| } | ||
|
|
||
| Ok(CentralizedKeygenOutput { | ||
| private_key: vec![0u8; 32], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private key must not be 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this
This reverts commit dfa1ccc.
|
frost_ed25519 is based on VSS, so we must change most of teddsa interfaces. Both keygen_1's output and keygen_2's output has all of key shares. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert this commit: be408b1? The existing serialization methods are not suitable for structure of our project . I'll add methods which are compatible with the existing architecture.
This reverts commit be408b1.
Pull Request
CONTRIBUTING.mdand followed the guidelines.Summary
Links (Issue References, etc, if there's any)