Add ability to run via containers #550
Conversation
|
Sorry, I can't accept this with the changes to the config file.
It is already hard enough to get some people to enter their username and
password correctly, if they have to fight around other bits of PHP code
they'll never manage it. That file has to stay as it is.
Why not just set docker up to use the defaults in that file? If someone is
using docker, there are either good enough to understand docker and can go
in and change things if they need to or they don't understand docker in
which case however you configure it, it doesn't matter as they will never
look inside it.
…On Mon, 3 Apr 2023, 17:02 Hoàng, ***@***.***> wrote:
Replaces #548 <#548>
------------------------------
You can view, comment on, or merge this pull request online at:
#550
Commit Summary
- 26112e2
<26112e2>
refactor: change quotes for literal strings
- 73e4c97
<73e4c97>
feat: enable reading variables from environment
- f88914f
<f88914f>
feat: enable containerization
- 5cdeb40
<5cdeb40>
docs: create new section for configurations
- 22b61c0
<22b61c0>
docs: add instructions for running via containers
File Changes
(6 files <https://github.com/digininja/DVWA/pull/550/files>)
- *A* .containerignore
<https://github.com/digininja/DVWA/pull/550/files#diff-2a6c4de4fd4d540549889f7844cbbdaf4babb4ae28c753020a21600201447210>
(6)
- *M* .gitignore
<https://github.com/digininja/DVWA/pull/550/files#diff-bc37d034bad564583790a46f19d807abfe519c5671395fd494d8cce506c42947>
(2)
- *A* Containerfile
<https://github.com/digininja/DVWA/pull/550/files#diff-5fcdf9b4580789697d834d1456a22bcfaa236d668fc180cad4775afc36ed5914>
(14)
- *M* README.md
<https://github.com/digininja/DVWA/pull/550/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5>
(53)
- *A* compose.yml
<https://github.com/digininja/DVWA/pull/550/files#diff-3493e6b5ddf34891e572f911db893efd9e46af828e011ea778a7c1eb64763588>
(34)
- *M* config/config.inc.php.dist
<https://github.com/digininja/DVWA/pull/550/files#diff-0c3c9e45825c5d2815e9dd22c7ed705c480f29117c1d64abb75b43576bcf1f59>
(112)
Patch Links:
- https://github.com/digininja/DVWA/pull/550.patch
- https://github.com/digininja/DVWA/pull/550.diff
—
Reply to this email directly, view it on GitHub
<#550>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA4SWOXNEJRAM22IU3CCLTW7LX7TANCNFSM6AAAAAAWRQYGW4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
I don't really see the problem here, as all of the configs have default values. With that said, this setup only needs a different Setting Docker to use only the defaults of the config file is possible, but it will require setting the network mode to |
|
Trust me, I've been doing support on this for at least the last five years
and the majority is issues raised are because people have messed up the
database connection, quite a few because they've mistyped things in that
file.
People will take the whole of that line, including the getenv and assume
it's the password, others will think something is wrong with the quotes and
quote the whole line, some will think they have to set the environment
variable and the default entry and ask questions.
You are thinking like someone who understands tech and I know it is a
really simple thing you've done, but it will mess people up I can guarantee
it.
…On Wed, 5 Apr 2023, 02:31 Hoàng, ***@***.***> wrote:
I don't really see the problem here, as all of the configs have default
values. With that said, this setup only needs a different DB_SERVER so
it's possible to have only the db_server read from the environment.
Setting Docker to use only the defaults of the config file is possible,
but it will require setting the network mode to host
<https://docs.docker.com/network/host/>. It can be predicted that there
will be port already in use errors for port 80 and 3306, but the exposed
ports can only be configured by the app within the containers, which will
require knowledge about Docker and the image being used. Doing this might
create more problems in the long run.
—
Reply to this email directly, view it on GitHub
<#550 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA4SWKZ2SH6MNB63IVXPVTW7TDONANCNFSM6AAAAAAWRQYGW4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
PTAL |
|
Will do later on.
I'm sorry to do this as I know your original way is the correct way, but
after having to explain to people that putting the username of root in the
config file but using dvwa when setting up the database means that things
don't line up, I just need to keep this as simple as possible.
…On Wed, 5 Apr 2023, 09:12 Hoàng, ***@***.***> wrote:
PTAL
—
Reply to this email directly, view it on GitHub
<#550 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA4SWMTCFZR24V3W563AODW7USQPANCNFSM6AAAAAAWRQYGW4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Asking for help on an unrelated three year old code change, this is an example of why any process has to be as simple and as thoroughly documented as possible. |
|
PTAL |
|
Away for the weekend, will look Monday.
…On Sat, 15 Apr 2023, 19:31 Hoàng, ***@***.***> wrote:
PTAL
—
Reply to this email directly, view it on GitHub
<#550 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA4SWMHJYTMNTSRWEMBTNTXBLSONANCNFSM6AAAAAAWRQYGW4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
The problem isn't that your instructions don't work, it is that they won't
work for everyone.
I've done videos and full walk through guides, and people still mess it up,
the less places they can go wrong, or the more places you can signpost them
to the right way to do something, the better.
…On Mon, 22 May 2023 at 14:11, Hoàng ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#550 (comment)>:
>
-This [video](https://youtu.be/Yzksa_WjnY0) walks you through the installation process for Windows but it should be similar for other OSs.
+0. Install [Docker](https://www.docker.com/) or your favorite container engine.
+1. Clone or download this repository and extract (see [Download](#download)).
+2. Open a terminal of your choice and change its working directory to `DVWA`.
+3. `docker compose up -d`
I have Docker installed via their convenience install script on Fedora
Server 38 and it works. I'll test this on Ubuntu VM soon.
—
Reply to this email directly, view it on GitHub
<#550 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA4SWLTHOXCDYH6FSVGZV3XHNQWZANCNFSM6AAAAAAWRQYGW4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Getting closer! |
|
Just use words, something like "notice that due to using containers, the
web server is not listening on the default port of 80, instead it has been
moved to 4280. For more information on why this is see ..." And give a
reference to non root containers not having access to low ports.
…On Mon, 5 Jun 2023, 04:46 Hoàng, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#550 (comment)>:
> +
+ Server: Docker Desktop 4.19.0 (106363)
+ Engine:
+ [...]
+ Version: 23.0.5
+ [...]
+
+ >>> docker compose version
+ Docker Compose version v2.17.3
+ ```
+
+2. Clone or download this repository and extract (see [Download](download)).
+3. Open a terminal of your choice and change its working directory to `DVWA`.
+4. `docker compose up -d`.
+
+DVWA is now available at `http://localhost:4280`.
How do you stress in markdown? Or raw html has to be used here?
—
Reply to this email directly, view it on GitHub
<#550 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA4SWLF4F6FXK5AWOQMSS3XJVJALANCNFSM6AAAAAAWRQYGW4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I want to tidy up the commit history once the PR is ready, so do give me some time then. |
|
I just followed all the instructions and it worked fine except one bug that was my fault, at some point I committed a change to the default config file which turned authentication off. I've turned that back on again so if you can make sure your default config gets updated with that change I think this is all ready to go. I know it has been a lot more work than you first thought, but I hope you see it is all worth it and gives something that is much easier to follow for complete beginners. Your next challenge, if you want, is to put some vulnerabilities in the container so users have even more to play with. |
I have rebased my branch to reflect this.
This is definitely more work than any contributions I have worked on. Still, the READMEs of other languages need some work, and there is not much I can do about that.
I don't know how well this fits in the scope of this project, but it's a good idea for future work. |
|
I have cleaned the commit history. It seems to have fixed the line ending conflict as well. |
|
<3 |
|
Thank you for all the hard work. The idea about vulns in the container was for a new project, not as part of this. |
|
I've just told the world about this. |
|
Great! Thank you for your work. |
Closes #386, replaces #548, related #384