search

TFNS / CTFNote

CTFNote is a collaborative tool aiming to help CTF teams to organise their work.
GNU General Public License v3.0
523 stars 61 forks source link

[Question] Will my changes be merged? #68

Closed JJ-8 closed 2 years ago

JJ-8 commented 3 years ago

Hello!

Amazing work with the new main branch! Really cool to see the Vue 3 / Quasar 2 / TypeScript migration and great to see that there is activity again!

While you were working on the graphql branch, I continued development on my own fork to get new features merged faster without disturbing your work on the new version. Since it is merged to master, I am willing to fork again and put my own changes on top of it. But my question is: should I create PRs for the changes or not? I really like to contribute to the upstream repo, but only if my changes are actually be merged (otherwise it only takes time to create branches and PRs).

It is about the following changes:

  1. Add player chips back to the task cards on the task overview.
    • This is because I really like to see in one glance who is working on what without having to hover over every task to see it. It is like the old behaviour of CTFNote where the names were visible at every task.
  2. Reload incoming CTFs when changed
    • This is still a bug in the new CTFNote, the view is not updated if there is a new CTF added. This is probably related to this issue.
  3. Websocket support for Hedgedoc
    • Instead of using HTTP long polling, websocket is much better and supported by Hedgedoc. It is only a little config switch in the nginx config.
  4. Deployment instructions for nginx
    • A specific config is required to use nginx with CTFNote. This is written in the README now. However, I changed some things: I bind to only localhost:8080 and instruct the user to use a HTTPS setup via nginx. So I don't expose CTFNote over insecure HTTP anymore. And there are some headers changed to work with Hedgedoc and HTTPS via CTFNote (these are some later commits in the repo).
  5. Add Hedgedoc authentication when authenticating with CTFNote (multiple commits so I can not link it).
    • This will auto create accounts and login the user in Hedgedoc so their username will be displayed in Hedgedoc. They will have a cursor with a username and a 'git blame' will be visible in the pad. The code consists of some hacky database manipulation to make it work, but it does work (requires connection to the Hedgedoc database).
  6. 'Past CTF' role in CTFNote
    • We introduced a new role in CTFNote: the 'Past CTF' role which means that a user should be invited to access upcoming CTFs but they have auto access to past CTFs. This is a role between guest and member. We found it very useful to grant to irregular users or if we use CTFNote with multiple teams.
  7. A CyberChef popup in the toolbar
    • There is a new button which allows users to quickly start multiple CyberChef instances in CTFNote. image

I am curious if you like these changes, if I should contribute them to upstream and if the changes will be merged in a reasonable amount of time.

XeR commented 3 years ago

Hey JJ-8,

Amazing work with the new main branch!

Thanks a lot !

I continued development on my own fork to get new features merged faster

Yes, we are aware of this. That's amazing ! I'm worried that the 9399a64f0e780a65adef8ea446353b6a4a411ae5 commit will make your life miserable… :-(

Add player chips back to the task cards on the task overview.

Sure, why not. Can you send us a screenshot to show us what it looks like ?

Reload incoming CTFs when changed

I thought we already fixed this bug. Sorry about that ! @B-i-t-K said he will take care of it

Websocket support for Hedgedoc Deployment instructions for nginx

Absolutely !

Add Hedgedoc authentication when authenticating with CTFNote

We've seen that one. We're a bit mixed about it. This is a good idea : being able to know who wrote something is very valuable. However, creating a new account every time looks a bit "fragile". As an operator, fixing bugs coming from this will be painful. Maybe there is a better way to do it ?

'Past CTF' role in CTFNote

Sure !

A CyberChef popup in the toolbar

We're also mixed on that : the popups are not synchronized between players, are they ? This looks like a whole new dependency for something that could be done simply by opening a new tab. Judging from your screenshot, you cannot copy/paste the link from one of your Cyberchef windows to a pad. Do users have to open a new tab, re-create the recipes and share the link ?

JJ-8 commented 3 years ago

I'm worried that the 9399a64 commit will make your life miserable… :-(

I think that I won't merge it but just rebase my merges on top of the latest master branch. Otherwise it will be too difficult.

Sure, why not. Can you send us a screenshot to show us what it looks like ?

The names are removed for privacy. This is a screenshot from the old UI of course. If you hover over the icon in the top right corner, then you will see the popup of active players also (I just haven't changed that because why would I). afbeelding

Maybe there is a better way to do it ?

It is indeed fragile if Hedgedoc changes its API. For registration / logging in, I just do a POST request to the correct endpoint, so I just use their API. However, deleting users as admin or changing the password requires direct database modifications since you are not authorized with the API. Without this, the CTFNote table could get out of sync with the Hedgedoc table and therefore authentication will fail with Hegdedoc. I don't know a better way than I have right now... It is quite stable and failed authentication with Hedgedoc won't block the authentication with CTFNote. I could add a setting to CTFNote to enable/disable the sync between Hedgedoc. If it causes any problems in the future, you can just disable it. Just for clarification, it is implemented in the following way:

  1. If a user is just registered to CTFNote, a registration request will also be done to Hedgedoc to add the user to Hedgedoc.
  2. For authentication: the login request will be done to Hedgedoc and if it response with the cookie, the cookie is send to the user. If there is no cookie, then probably the user did not have an account yet on Hedgedoc, so we register and authenticate again.
  3. If the user changes the password, the password field in Hedgedoc will be manually updated with a database operation using the exact same hashing config as Hedgedoc (same for updating the username).
  4. If the admin/user deletes the account, then this is done with a database operation too. Hedgedoc automatically removes every references to the user from any note.

You can read the code here.

We're also mixed on that : the popups are not synchronized between players, are they ?

No, they are just local popups. I was thinking about this, but if two users changes the CyberChef at the same time, then it would be impossible to know what to do. However, it doesn't introduce a huge new dependency. It only uses https://nextapps-de.github.io/winbox/ to iFrame the CyberChef website.

Judging from your screenshot, you cannot copy/paste the link from one of your Cyberchef windows to a pad.

I completely forgot about that! xD That is not useful at all. Maybe I can add a button to copy the link to your clipboard. That is indeed a very important feature. The reason that I created this popup, is because I often have like 3-5 CyberChef tabs open with different recipes and now I can use the CTFNote tab instead. It is just for efficiency.

I forgot to mention one more feature that I was working on recently: adding DestructiveFarm to the deployment for CTFNote to easily host infrastructure for Attack/Defence CTFs. While it is a bit out of scope for the CTFNote project, I still want a way to easily host DestructiveFarm without registering a new domain or use another port (and have to reconfigure nginx to use HTTPS). What do you think about this? I understand if this is something you don't like. I can make a separate docker-compose.yml copy that will include this, so the normal config won't deploy DestructiveFarm. Is this something that you would merge? It requires an external repository, but I can also just add instructions to do a clone manually if this is not something that you want to add to the project.

XeR commented 3 years ago

Add player chips back to the task cards on the task overview

It was removed before we added the "dense" and "ultradense" themes. If you can add it only on the "classic" theme, we see no reasons not to merge it.

I don't know a better way than I have right now

The "cleanest" way seems to be implementing an OAuth2 provider. This looks like a complex task, but it would have the advantage of being able to work on a different domain. Nothing set in stone though ! We like the idea of having an account on Hedgedoc, this will be implemented in the future.

A CyberChef popup in the toolbar

Ah, so your workflow is to open a new pop-up, try some recipes and keep track of what worked/did not work on a pad ? I understand now. We'll talk about it. The current focus is fixing bugs so I don't think we'll add it on the short term.

adding DestructiveFarm to the deployment for CTFNote

It's the first time I hear about DestructiveFarm. We think this is not in the scope of CTFNote. Sorry.

As for the other patches (3, 4), you should be able to branch from main and cherry-pick them to keep the metadata. :-)