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.

Sign up for GitHub

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

Jump to bottom

improve(feat): Imagebuild and Imagesize #876

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

improve(feat): Imagebuild and Imagesize #876

wants to merge 6 commits into from

Conversation

AndreasHeine
Copy link
Owner

No description provided.

@AndreasHeine AndreasHeine marked this pull request as draft September 30, 2024 14:40
GoetzGoerisch
GoetzGoerisch reviewed Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@GoetzGoerisch GoetzGoerisch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you think of a healthcheck of the running application in the container please?

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@AndreasHeine
Copy link
Owner Author

@GoetzGoerisch

 RUN apk --no-cache add \
      openssl=3.0.14-r0 \
      python3=3.10.14-r1 \
      make=4.3-r1 \
      g++=12.2.1_git20220924-r4 \
      gcc=12.2.1_git20220924-r4

this makes the image double it size!

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 1, 2024
View reviewed changes
src/user.ts Fixed Show fixed Hide fixed
src/utils/userfile.ts Fixed Show fixed Hide fixed
@AndreasHeine AndreasHeine marked this pull request as ready for review October 1, 2024 07:49
@AndreasHeine
Copy link
Owner Author

i had to remove bcrypt because of its deps to python and openssl... it add extra ~400-500 MB to each image!

https://www.npmjs.com/package/bcrypt

Dependencies

    NodeJS
    node-gyp
    Please check the dependencies for this tool at: https://github.com/nodejs/node-gyp
    Windows users will need the options for c# and c++ installed with their visual studio instance.
    Python 2.x/3.x
    OpenSSL - This is only required to build the bcrypt project if you are using versions <= 0.7.7. Otherwise, we're using the builtin node crypto bindings for seed data (which use the same OpenSSL code paths we were, but don't have the external dependency).

@AndreasHeine AndreasHeine changed the title Update Dockerfile improve(feat): Imagebuild and Imagesize Oct 1, 2024
@AndreasHeine
Copy link
Owner Author

at this stage we just connect and disconnect!
maybe in the future we need to create a session and read something... not sure what we should test @GoetzGoerisch

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 1, 2024
View reviewed changes
src/user.ts
callback(null, false);
}
});
const hash = createHash("sha512").update(password).digest("hex")

Check failure

Code scanning / CodeQL

Use of password hash with insufficient computational effort High

Password from
an access to password
Loading
is hashed insecurely.

Copilot Autofix AI 3 months ago

To fix the problem, we need to replace the use of the sha512 hashing algorithm with a more secure password hashing algorithm, such as bcrypt. This will involve:

  1. Importing the bcrypt library.
  2. Modifying the isValidUserAsync function to use bcrypt for password hashing and comparison.
  3. Ensuring that the bcrypt library is installed as a dependency.
Suggested changeset 2
src/user.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/src/user.ts b/src/user.ts
--- a/src/user.ts
+++ b/src/user.ts
@@ -22,3 +22,3 @@
 import { green, red } from './utils/log';
-import { createHash } from 'crypto';
+import * as bcrypt from 'bcrypt';
 
@@ -54,4 +54,4 @@
   if (user) {
-    const hash = createHash("sha512").update(password).digest("hex")
-    if (hash === user.password) {
+    const isMatch = bcrypt.compareSync(password, user.password);
+    if (isMatch) {
       green(` User:'${user.username}' logged in as '${user.roles}'! `);
EOF
@@ -22,3 +22,3 @@
import { green, red } from './utils/log';
import { createHash } from 'crypto';
import * as bcrypt from 'bcrypt';

@@ -54,4 +54,4 @@
if (user) {
const hash = createHash("sha512").update(password).digest("hex")
if (hash === user.password) {
const isMatch = bcrypt.compareSync(password, user.password);
if (isMatch) {
green(` User:'${user.username}' logged in as '${user.roles}'! `);
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/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -33,3 +33,4 @@
     "node-opcua-server-discovery": "2.133.0",
-    "yargs": "17.7.2"
+    "yargs": "17.7.2",
+    "bcrypt": "^5.1.1"
   },
EOF
@@ -33,3 +33,4 @@
"node-opcua-server-discovery": "2.133.0",
"yargs": "17.7.2"
"yargs": "17.7.2",
"bcrypt": "^5.1.1"
},
This fix introduces these dependencies
Package Version Security advisories
bcrypt (npm) 5.1.1 None
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
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
src/utils/userfile.ts
@@ -32,7 +32,7 @@
this.userList.push(Object.freeze({
username: user.username,
roles: user.roles,
password: hashSync(user.password, genSaltSync())
password: createHash("sha512").update(user.password).digest("hex")

Check failure

Code scanning / CodeQL

Use of password hash with insufficient computational effort High

Password from
an access to password
Loading
is hashed insecurely.

Copilot Autofix AI 3 months ago

To fix the problem, we need to replace the use of the sha512 hashing algorithm with a more secure password hashing scheme. The bcrypt library is a good choice for this purpose as it is specifically designed for hashing passwords securely.

  • First, we need to install the bcrypt library if it is not already installed.
  • Then, we need to import the bcrypt library in the file.
  • Finally, we need to replace the createHash("sha512").update(user.password).digest("hex") line with bcrypt.hashSync(user.password, saltRounds), where saltRounds is a parameter that defines the computational cost of the hashing process.
Suggested changeset 2
src/utils/userfile.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/src/utils/userfile.ts b/src/utils/userfile.ts
--- a/src/utils/userfile.ts
+++ b/src/utils/userfile.ts
@@ -15,3 +15,3 @@
 import { writeFileSync } from 'fs'
-import { createHash } from 'crypto';
+import * as bcrypt from 'bcrypt';
 
@@ -34,3 +34,3 @@
             roles: user.roles,
-            password: createHash("sha512").update(user.password).digest("hex")
+            password: bcrypt.hashSync(user.password, 10)
         }))
EOF
@@ -15,3 +15,3 @@
import { writeFileSync } from 'fs'
import { createHash } from 'crypto';
import * as bcrypt from 'bcrypt';

@@ -34,3 +34,3 @@
roles: user.roles,
password: createHash("sha512").update(user.password).digest("hex")
password: bcrypt.hashSync(user.password, 10)
}))
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/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -33,3 +33,4 @@
     "node-opcua-server-discovery": "2.133.0",
-    "yargs": "17.7.2"
+    "yargs": "17.7.2",
+    "bcrypt": "^5.1.1"
   },
EOF
@@ -33,3 +33,4 @@
"node-opcua-server-discovery": "2.133.0",
"yargs": "17.7.2"
"yargs": "17.7.2",
"bcrypt": "^5.1.1"
},
This fix introduces these dependencies
Package Version Security advisories
bcrypt (npm) 5.1.1 None
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
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
Copy link
Owner Author

@AndreasHeine AndreasHeine Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot ... i dont want bcrypt... grrr!

GoetzGoerisch
GoetzGoerisch approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@GoetzGoerisch GoetzGoerisch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase and decide what to do with the cryptography feedback from GitHub.

Send you two possible alternatives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GoetzGoerisch GoetzGoerisch GoetzGoerisch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AndreasHeine @GoetzGoerisch