Use random generated key

[window9u]

What this PR does / why we need it

This PR introduces a new method for generating API keys using short UUID instead of the current time-based approach. This change aims to enhance security and reduce the predictability of our API keys.

Which issue(s) this PR fixes:

Fixes #

Key Generation Options

1. Custom Charset [0-9a-v]

• Description: Uses a custom charset of 32 characters (0-9 and a-v)
• Process: Generates random bytes and maps them to the charset
• Output: String of specified length
• Example: "f3c5h9m937etoad7l48ioc5oo1jr5p1h"
• Pros: Utilizes the same charset used in xid ([0-9a-v]).
• Cons: This encoding method may be less efficient or use more space.

const charset = "0123456789abcdefghijklmnopqrstuv"
randomBytes := make([]byte, length)
_, err := rand.Read(randomBytes)
result := make([]byte, length)
for i := 0; i < length; i++ {
    result[i] = charset[randomBytes[i]%uint8(len(charset))]
}

2. Base64 Encoding

• Description: Generates random bytes and encodes using base64 URL-safe encoding
• Output: Base64 encoded string
• Example: "zehf_23Bg2vrYk4VMZvgiQDqcgRgLRDcLBx3IvONyTY="
• Pros: Easily allows setting the length of the key.
• Cons: Requires error handling, which makes the code more complex.

bytes := make([]byte, length)
_, err := rand.Read(bytes)
if err != nil {
    return "", fmt.Errorf("failed to generate random bytes: %w", err)
}
return base64.URLEncoding.EncodeToString(bytes), nil

3. ShortUUID(selected option)

• Description: Uses the shortuuid library
• Process: Generates a short UUID
• Note: Ignores the length parameter
• Example: "cqz7HZQp7VDm4Lg5otd3ZE"
• Pros:
  • The code is the cleanest.
  • Compresses the UUID using base57, reducing the string length.
• Cons: Cannot set the length of the key.
• Selected reason: I believe this option strikes a good balance between security and efficiency, and it offers the simplest and cleanest code.

shortuuid.New()

4. Standard UUID

• Description: Uses the standard UUID library
• Process: Generates a standard UUID (version 4)
• Example: "3023ef2e-3a04-404d-9f84-c08d77f43ab0"
• Cons: Does not reduce the string length as effectively as the ShortUUID method, which compresses using base57.

uuid.NewString()

Notes for Reviewers

1. Provide new API: Consider creating an issue to implement an API for key refreshment.
2. Uniqueness: This approach does not ensure absolute uniqueness, but the probability of collision is extremely low. See UUID comment for details. MongoDB uniqueness constraint ensure this uniqueness comment

[coderabbitai]

Walkthrough

The changes enhance error handling and functionality in several database-related functions. The ensureDefaultProjectInfo and CreateProjectInfo methods now include error checks when creating ProjectInfo instances. Additionally, the NewProjectInfo function has been updated to generate random keys for PublicKey and SecretKey, returning an error if key generation fails. The test suite has also been modified to reflect these changes, ensuring that errors during ProjectInfo creation are properly asserted.

Changes

Files	Change Summary
server/backend/database/memory/database.go	Enhanced error handling in ensureDefaultProjectInfo and CreateProjectInfo functions.
server/backend/database/mongo/client.go	Enhanced error handling in ensureDefaultProjectInfo and CreateProjectInfo methods of Client struct.
server/backend/database/project_info.go	Updated NewProjectInfo to generate random keys and return an error; added GenerateRandomKey function.
server/backend/database/project_info_test.go	Updated TestProjectInfo to include error checking during ProjectInfo instantiation.
go.mod	Added dependency on github.com/lithammer/shortuuid/v4 and removed indirect dependency on github.com/stretchr/objx.

Sequence Diagram(s)

sequenceDiagram
    participant DB
    participant Client
    participant ProjectInfo

    DB->>ProjectInfo: NewProjectInfo(name, owner, threshold)
    alt Error Occurs
        ProjectInfo-->>DB: return error
    else Success
        ProjectInfo-->>DB: return ProjectInfo instance
    end
    DB->>Client: ensureDefaultProjectInfo()
    Client->>DB: CreateProjectInfo()
    alt Error Occurs
        DB-->>Client: return error
    else Success
        DB-->>Client: return ProjectInfo instance
    end

Loading

Poem

🐇 In the garden of code, where errors once lay,
New keys are now born, brightening the day.
With checks in the functions, they dance and they play,
Robustness and joy, in a hop and a sway!
So here’s to the changes, let’s cheer and convey,
A future of coding, where bugs fade away! 🌼

---

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 27ba904 and 2b4df0b.

Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

Files selected for processing (2)

• go.mod (1 hunks)
• server/backend/database/project_info.go (2 hunks)

Files skipped from review as they are similar to previous changes (1)

• server/backend/database/project_info.go

Additional comments not posted (1)

go.mod (1)

15-15: LGTM!

The addition of the github.com/lithammer/shortuuid/v4 package at version v4.0.0 aligns with the PR objective of introducing a new method for generating API keys using random values. This package provides functionality for generating concise, unique identifiers, which can be utilized for the new key generation approach.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  -- I pushed a fix in commit <commit_id>, please review it.
  -- Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  -- @coderabbitai generate unit testing code for this file.
  -- @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  -- @coderabbitai generate interesting stats about this repository and render them as a table.
  -- @coderabbitai read src/utils.ts and generate unit testing code.
  -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  -- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (2)

server/backend/database/project_info.go (2)

72-88: Consider migrating existing time-based keys.

The PR introduces a new method for generating keys but does not address the migration of existing time-based keys. Consider adding a migration plan to ensure all keys are using the new secure method.

---

72-88: Create a separate issue for implementing the API gateway.

The PR objectives mention creating an issue to implement an API gateway for key refreshment. Please create a separate issue to track this work.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0871fac and 9d5220f.

Files selected for processing (4)

• server/backend/database/memory/database.go (2 hunks)
• server/backend/database/mongo/client.go (2 hunks)
• server/backend/database/project_info.go (3 hunks)
• server/backend/database/project_info_test.go (1 hunks)

Additional context used

golangci-lint

server/backend/database/project_info.go

158-158: error returned from external package is unwrapped: sig: func crypto/rand.Read(b []byte) (n int, err error)

(wrapcheck)

GitHub Check: build

server/backend/database/project_info.go

[failure] 158-158:
error returned from external package is unwrapped: sig: func crypto/rand.Read(b []byte) (n int, err error) (wrapcheck)

Additional comments not posted (7)

server/backend/database/project_info_test.go (1)

32-33: LGTM!

The change to include error handling when creating the project object is a good improvement. It ensures that any issues during the instantiation of project are caught and reported, enhancing the robustness and reliability of the test.

server/backend/database/project_info.go (2)

72-88: LGTM!

The changes to NewProjectInfo look good. Generating random keys for PublicKey and SecretKey fields enhances security by replacing the time-based approach. The error handling ensures that the caller is notified if key generation fails.

---

154-161: LGTM!

The new GenerateRandomKey function looks good. It generates a secure random base64-encoded string of the specified length using the crypto/rand package. The error handling ensures that the caller is notified if random byte generation fails.

Tools

golangci-lint

158-158: error returned from external package is unwrapped: sig: func crypto/rand.Read(b []byte) (n int, err error)

(wrapcheck)

GitHub Check: build

[failure] 158-158:
error returned from external package is unwrapped: sig: func crypto/rand.Read(b []byte) (n int, err error) (wrapcheck)

server/backend/database/memory/database.go (2)

203-206: LGTM!

The added error handling improves the robustness of the function by ensuring that any errors during project creation are properly propagated. This change aligns with best practices for error handling in Go.

---

239-242: LGTM!

The added error handling improves the robustness of the function by ensuring that any errors during project creation are properly propagated. This change aligns with best practices for error handling in Go.

server/backend/database/mongo/client.go (2)

168-171: LGTM!

The added error handling for database.NewProjectInfo improves the robustness of the code. It ensures that if an error occurs during the creation of the default project info, it is properly propagated with additional context. This prevents the use of potentially uninitialized or invalid ProjectInfo objects.

---

212-215: LGTM!

Similar to the changes in ensureDefaultProjectInfo, the added error handling for database.NewProjectInfo enhances the robustness of the CreateProjectInfo function. It ensures that if an error occurs during the creation of a new project info, it is properly propagated with additional context. This prevents the use of potentially uninitialized or invalid ProjectInfo objects.

[krapie]

Thank you for your contribution.
Below are some comments regarding this change.

Reason for this change?

// TODO(hackerwins): Use random generated Key.

Are we changing because current key generation xid uses the Mongo Object ID algorithm and we don't want to be predictable, just like @window9u explained? Knowing the exact reason will help us determine if this approach is suitable. @hackerwins

API Key rolling

API Gateway: Consider creating an issue to implement an API gateway for key refreshment.

I agree with your opinion of having API for rolling (refreshing) API key by various cases like key exposure. But what does it mean by implementing API Gateway?

[window9u]

@krapie

1. I noticed that the public and private keys are almost identical except for the last word. Public key is shared with normal client, so it is easy to guess private key. You can also verify this in your dashboard. This is why I attempted to change the xid to a randomly generated key.

2. I misused the term "gateway." As you correctly understood, I meant to add a key rolling API. I think we should reset the previously generated key using the xid.

[krapie]

@sejongk Is there any constraints when we use UUID as project key considering our MongoDB environment especially for indexing and sharding?

[sejongk]

Regarding the uniqueness of UUID, the public_key and secret_key in project_info are ensured to be unique in MongoDB thanks to the use of a uniqueness constraint.

yorkie/server/backend/database/mongo/indexes.go

Line 73 in 0871fac

Options: options.Index().SetUnique(true),

[sejongk]

@sejongk Is there any constraints when we use UUID as project key considering our MongoDB environment especially for indexing and sharding?

@krapie In the current MongoDB sharding settings, combinations of project_id (using MongoDB ObjectID) and doc_key/client_key (a user-assigned slug with 4 to 120 characters) are used as shard keys. Therefore, I believe there are no side effects on the MongoDB sharding setup.

[krapie]

Since we do not put any information in our API key, I think (shorten) UUID is good approach for generating API key in terms of its randomness, uniqueness.

I was little worried about indexing of this API key in terms of retrieving Project, since we retrieve Project every time the API call is made. But since we are using cache while retrieving this, it will have little impact on the performance. Reference: https://github.com/icamys/mongo-ULID-UUID-ObjectID Single query (not range) that we use will not affect performance.

@window9u Could you create subsequence issue or PR for providing API for rolling these API keys?

[window9u]

@krapie

Thank you for reviewing my PR. I've created the issue!!