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

Updated Docker Build & Vulnerability Patching #240

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

NOXCIS
Copy link

@NOXCIS NOXCIS commented Dec 31, 2023

@jrmi
Copy link
Collaborator

jrmi commented Dec 31, 2023

@jrmi

Perfect, will review it ASAP.

Copy link
Collaborator

@jrmi jrmi left a comment

Choose a reason for hiding this comment

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

Great job Noxcis. I like the multistage docker image.

I left a few comments, let me know what you think.

Dockerfile Outdated Show resolved Hide resolved
@@ -14,4 +14,4 @@
"background_color": "#ffffff",
"scope": "/",
"description": "Secure and encrypted web chat with Darkwire.io"
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this change is unnecessary ;-)

Copy link
Author

Choose a reason for hiding this comment

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

Still haven't gotten around to configure my IDE, spends time time fighting docker engine.

start.sh Outdated
openssl genpkey -algorithm RSA -out "$key_file"

# Generate certificate signing request (CSR)
openssl req -new -key "$key_file" -out "$csr_file" -subj "/C=US/ST=FL/L=Miami/O=NoxCorp/OU=GhostWorks/CN=Noxcis"
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be the "subject" should be more... generic? May be something that can be configured with the env if needed with the previous value as default, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I agree any ideas on how to implement?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use define a new env var like: CSR_SUBJECT and use it in this script? What do you think?

client/.env.dist Outdated
@@ -1,14 +0,0 @@
# Api settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you remove this file (and the one from the server) ? I think it still a good idea to have a template when in development or may be if someone wants a custom install without docker for instance, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

hmm, great point!

start.sh Outdated
set_env &&
# Start your application
generate_self_signed_ssl &&
nginx &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't install Nginx in the Darkwire image. I think it's a good idea to keep things separated. Imagine someone that already have a docker compose stack with an existing Nginx, or someone who prefers Traefik. Adding Nginx doesn't break things, but it adds an extra overhead in most situation. As consequence, I would just keep the yarn start here. However, I think it makes sense to update the docker compose file to use nginx as reverse proxy. 

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I agree will reserve for my hard-fork meant to run with my project over here Wiregate.

client/src/stylesheets/app.sass Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants