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

Kubero V3 refactoring #619

Draft
wants to merge 67 commits into
base: main
Choose a base branch
from
Draft

Kubero V3 refactoring #619

wants to merge 67 commits into from

Conversation

mms-gianni
Copy link
Member

@mms-gianni mms-gianni commented Jan 31, 2025

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Template (non-breaking change which adds a template)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • I've built the image and tested it on a kubernetes cluster

Test Configuration:

  • Operator Version:
  • Kubernetes Version:
  • Kubero CLI Version (if applicable):

Checklist:

  • I removed unnecessary debug logs
  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I documented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@mms-gianni mms-gianni marked this pull request as draft January 31, 2025 13:34
@mms-gianni mms-gianni self-assigned this Feb 3, 2025
@mms-gianni mms-gianni added the in progress working on it label Feb 3, 2025
@mms-gianni mms-gianni linked an issue Feb 3, 2025 that may be closed by this pull request
@mms-gianni
Copy link
Member Author

Progress

image

res.send(template);
} catch (err) {
this.logger.error(err);
res.status(500).send(err);

Check warning

Code scanning / CodeQL

Information exposure through a stack trace Medium

This information exposed to the user depends on
stack trace information
.

Copilot Autofix AI 6 days ago

To fix the problem, we need to ensure that the stack trace and any sensitive information contained in the error object are not exposed to the end user. Instead, we should log the error details on the server and send a generic error message to the user. This can be achieved by modifying the catch block to log the error and send a generic message.

Suggested changeset 1
server-refactored-v3/src/templates/templates.controller.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/src/templates/templates.controller.ts b/server-refactored-v3/src/templates/templates.controller.ts
--- a/server-refactored-v3/src/templates/templates.controller.ts
+++ b/server-refactored-v3/src/templates/templates.controller.ts
@@ -40,4 +40,4 @@
     } catch (err) {
-      this.logger.error(err);
-      res.status(500).send(err);
+      this.logger.error('Exception occurred', err.stack);
+      res.status(500).send('An error occurred while processing your request.');
     }
EOF
@@ -40,4 +40,4 @@
} catch (err) {
this.logger.error(err);
res.status(500).send(err);
this.logger.error('Exception occurred', err.stack);
res.status(500).send('An error occurred while processing your request.');
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
res.send(template);
} catch (err) {
this.logger.error(err);
res.status(500).send(err);

Check warning

Code scanning / CodeQL

Exception text reinterpreted as HTML Medium

Exception text
is reinterpreted as HTML without escaping meta-characters.

Copilot Autofix AI 6 days ago

To fix the problem, we need to ensure that the error message is properly sanitized or escaped before being sent in the response. This can be achieved by using a library like he (HTML entities) to escape the error message. This will prevent any malicious content from being interpreted as HTML.

  1. Install the he library to handle HTML escaping.
  2. Import the he library in the templates.controller.ts file.
  3. Use the he.escape function to escape the error message before sending it in the response.
Suggested changeset 2
server-refactored-v3/src/templates/templates.controller.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/src/templates/templates.controller.ts b/server-refactored-v3/src/templates/templates.controller.ts
--- a/server-refactored-v3/src/templates/templates.controller.ts
+++ b/server-refactored-v3/src/templates/templates.controller.ts
@@ -11,3 +11,3 @@
 import { OKDTO } from 'src/shared/dto/ok.dto';
-
+import * as he from 'he';
 @Controller({ path: 'api/templates', version: '1' })
@@ -41,3 +41,3 @@
       this.logger.error(err);
-      res.status(500).send(err);
+      res.status(500).send(he.escape(err.toString()));
     }
EOF
@@ -11,3 +11,3 @@
import { OKDTO } from 'src/shared/dto/ok.dto';

import * as he from 'he';
@Controller({ path: 'api/templates', version: '1' })
@@ -41,3 +41,3 @@
this.logger.error(err);
res.status(500).send(err);
res.status(500).send(he.escape(err.toString()));
}
server-refactored-v3/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/package.json b/server-refactored-v3/package.json
--- a/server-refactored-v3/package.json
+++ b/server-refactored-v3/package.json
@@ -54,3 +54,4 @@
     "sshpk": "^1.18.0",
-    "yaml": "^2.7.0"
+    "yaml": "^2.7.0",
+    "he": "^1.2.0"
   },
EOF
@@ -54,3 +54,4 @@
"sshpk": "^1.18.0",
"yaml": "^2.7.0"
"yaml": "^2.7.0",
"he": "^1.2.0"
},
This fix introduces these dependencies
Package Version Security advisories
he (npm) 1.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options

const password = crypto
.createHmac('sha256', process.env.KUBERO_SESSION_KEY)
.update(pass)

Check failure

Code scanning / CodeQL

Use of password hash with insufficient computational effort High

Password from
an access to password
is hashed insecurely.
Password from
an access to password
is hashed insecurely.
Password from
an access to password
is hashed insecurely.

Copilot Autofix AI 6 days ago

To fix the problem, we need to replace the current password hashing mechanism with a more secure one, such as bcrypt. This will ensure that the password hashing process is computationally intensive, making it more resistant to brute-force attacks.

  1. Install the bcrypt library if it is not already installed.
  2. Import the bcrypt library in the auth.service.ts file.
  3. Replace the existing SHA-256 hashing code with bcrypt hashing.
  4. Update the password comparison logic to use bcrypt's comparison function.
Suggested changeset 1
server-refactored-v3/src/auth/auth.service.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/src/auth/auth.service.ts b/server-refactored-v3/src/auth/auth.service.ts
--- a/server-refactored-v3/src/auth/auth.service.ts
+++ b/server-refactored-v3/src/auth/auth.service.ts
@@ -13,3 +13,3 @@
 import { JwtService } from '@nestjs/jwt';
-import * as crypto from 'crypto';
+import * as bcrypt from 'bcrypt';
 
@@ -36,7 +36,4 @@
 
-      const password = crypto
-        .createHmac('sha256', process.env.KUBERO_SESSION_KEY)
-        .update(pass)
-        .digest('hex');
-      if (user.password === password) {
+      const passwordMatch = await bcrypt.compare(pass, user.password);
+      if (passwordMatch) {
         const { password, ...result } = user;
EOF
@@ -13,3 +13,3 @@
import { JwtService } from '@nestjs/jwt';
import * as crypto from 'crypto';
import * as bcrypt from 'bcrypt';

@@ -36,7 +36,4 @@

const password = crypto
.createHmac('sha256', process.env.KUBERO_SESSION_KEY)
.update(pass)
.digest('hex');
if (user.password === password) {
const passwordMatch = await bcrypt.compare(pass, user.password);
if (passwordMatch) {
const { password, ...result } = user;
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
// decode the base64 encoded URL
const templateUrl = Buffer.from(templateB64, 'base64').toString('ascii');

const template = await axios.get(templateUrl).catch((err) => {

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
of this request depends on a
user-provided value
.

Copilot Autofix AI 6 days ago

To fix the SSRF vulnerability, we need to validate and restrict the user input to ensure it only allows safe and intended URLs. One way to achieve this is by using an allow-list of trusted base64 encoded URLs. This approach ensures that only predefined and safe URLs can be used in the axios.get request.

  1. Create an allow-list of trusted base64 encoded URLs.
  2. Validate the templateB64 parameter against this allow-list before decoding and using it in the request.
  3. If the templateB64 parameter is not in the allow-list, throw an error.
Suggested changeset 1
server-refactored-v3/src/templates/templates.service.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/src/templates/templates.service.ts b/server-refactored-v3/src/templates/templates.service.ts
--- a/server-refactored-v3/src/templates/templates.service.ts
+++ b/server-refactored-v3/src/templates/templates.service.ts
@@ -7,2 +7,7 @@
   private YAML = require('yaml');
+  private readonly allowedTemplates = [
+    'aHR0cHM6Ly9leGFtcGxlLmNvbS90ZW1wbGF0ZTE=', // base64 for 'https://example.com/template1'
+    'aHR0cHM6Ly9leGFtcGxlLmNvbS90ZW1wbGF0ZTI='  // base64 for 'https://example.com/template2'
+  ];
+
   constructor() {}
@@ -10,2 +15,7 @@
   async getTemplate(templateB64: string) {
+    // validate the base64 encoded URL
+    if (!this.allowedTemplates.includes(templateB64)) {
+      throw new Error('Invalid template URL');
+    }
+
     // decode the base64 encoded URL
EOF
@@ -7,2 +7,7 @@
private YAML = require('yaml');
private readonly allowedTemplates = [
'aHR0cHM6Ly9leGFtcGxlLmNvbS90ZW1wbGF0ZTE=', // base64 for 'https://example.com/template1'
'aHR0cHM6Ly9leGFtcGxlLmNvbS90ZW1wbGF0ZTI=' // base64 for 'https://example.com/template2'
];

constructor() {}
@@ -10,2 +15,7 @@
async getTemplate(templateB64: string) {
// validate the base64 encoded URL
if (!this.allowedTemplates.includes(templateB64)) {
throw new Error('Invalid template URL');
}

// decode the base64 encoded URL
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
);
password = crypto
.createHmac('sha256', process.env.KUBERO_SESSION_KEY)
.update(password)

Check failure

Code scanning / CodeQL

Use of password hash with insufficient computational effort High

Password from
an access to password
is hashed insecurely.
Password from
an access to password
is hashed insecurely.

Copilot Autofix AI 6 days ago

To fix the problem, we should replace the use of crypto.createHmac('sha256', ...) with a more secure password hashing scheme such as bcrypt. This will ensure that the password hashing process requires significant computational effort, making it more resistant to brute-force attacks.

The best way to fix the problem without changing existing functionality is to use the bcrypt library to hash the passwords. We will need to import the bcrypt library, update the password hashing logic to use bcrypt.hashSync, and ensure that the salt is properly generated and used.

Suggested changeset 1
server-refactored-v3/src/users/users.service.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/server-refactored-v3/src/users/users.service.ts b/server-refactored-v3/src/users/users.service.ts
--- a/server-refactored-v3/src/users/users.service.ts
+++ b/server-refactored-v3/src/users/users.service.ts
@@ -3,3 +3,3 @@
 dotenv.config();
-import * as crypto from 'crypto';
+import * as bcrypt from 'bcrypt';
 
@@ -45,6 +45,4 @@
           );
-          password = crypto
-            .createHmac('sha256', process.env.KUBERO_SESSION_KEY)
-            .update(password)
-            .digest('hex');
+          const saltRounds = 10;
+          password = bcrypt.hashSync(password, saltRounds);
         }
EOF
@@ -3,3 +3,3 @@
dotenv.config();
import * as crypto from 'crypto';
import * as bcrypt from 'bcrypt';

@@ -45,6 +45,4 @@
);
password = crypto
.createHmac('sha256', process.env.KUBERO_SESSION_KEY)
.update(password)
.digest('hex');
const saltRounds = 10;
password = bcrypt.hashSync(password, saltRounds);
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress working on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Announcing Kubero v3 (Refactoring)
1 participant