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

Add a recipient metadata/field for additional authentication via reverse-proxy #1096

Open
C-Duv opened this issue Jun 9, 2023 · 4 comments

Comments

@C-Duv
Copy link

C-Duv commented Jun 9, 2023

For an internal use case I need to add a "recipient" metadata to pastes.

The goal was to:

  • Use PrivateBin to share a secret with someone.
  • Send the paste URL via e-mail.
  • Rely on third-party SSO to make sure only the correct person can access the paste (e-mails are not that safe).

(for about 150 non-tech users)

So I have made a small hack on a fork: https://github.com/C-Duv/PrivateBin/tree/feat/add_recipient_to_documents

It adds a recipient metadata in adata (at index 4) and the corresponding text field on the web GUI.
If set, the instance checks if recipient matches when displaying the paste.

It works as follows:

  1. Access the "new paste" page of the instance
  2. Paste the text as usual
  3. Set a "recipient" (more on this below)
    (If no recipient is set, the behavior of the application is unchanged: like if my changes don't exist)
  4. Send the paste URL to the person via e-mail, QRcode, etc.
  5. The PrivateBin instance is behind a reverse-proxy that adds an authentication layer (using .htpasswd, OpenID Connect SSO, LDAP, whatever, …)
  6. When accessing the Paste, the PrivateBin instance checks if login (passed by the reverse-proxy, via an HTTP Header) matches the one of the paste (exactly like the one set at step 1.):
    • If it does: The paste is displayed to the user
    • If it does not: The "not found" page is shown

So, the "recipient" value is dependent on the authentication system used.

What do you think of such feature? How could it be more nicely integrated into PrivateBin? Using a plugin (is there even a plugin system)?

@elrido
Copy link
Contributor

elrido commented Jun 10, 2023

Thank you for sharing your work. I'm always happy to hear that this tool gets real-world use and to hear about usage scenarios that I hadn't considered before.

Q: Is there a plugin system in PrivateBin?
A: Not explicitly and not on the client side. There is the template system for the server side display logic. On the client side, PrivateBin uses the jQuery.PrivateBin namespace to reduce pollution of the global namespace. One can use this and the javascripts language features to monkey patch the public methods or replace whole objects with custom, extended ones, if private logic needs to be extended. This works, but clearly is not very elegant, nor is it very maintainable in the long run, as new versions may change these APIs.

Q: What do you think of such feature?
A: I do like that you made the effort:

  1. ...of extending the authenticated data (adata) at the end, as this makes this backwards compatible with other tooling, that will simply ignore the extra field. Storing it in adata also means that the value can't be manipulated after creation without the recipient noticing (meaning the decryption fails).
  2. ...making this a configurable item, and off by default. It would therefore not impact existing users that are not using the feature, if they'd upgraded to this code base.
  3. ...keeping the authentication and account management outside of the PrivateBin codebase. Given that it just adds an extra string to the paste and defers to the webserver what to do with it, if anything, it is also a pretty flexible solution. Instead of checking for user names, the webserver could check if the current user is member of a group matching that string. It could also be used in other ways, where the webserver changes the response when the extra string is present or matches a certain value, for example to add a per paste, customized notice message (though this extra string is not encrypted and is being returned visitors, even if they fail to decrypt the paste).

As such, I could see this being integrated into the main project, as an optional feature that requires extra configuration steps in the webserver to make use of. These extra steps should be described on a dedicated document, for example a wiki page mirroring the Restrict upload using NGINX one.

What do other folks think about adding this feature? What risks would this introduce, apart from increasing complexity?

@C-Duv
Copy link
Author

C-Duv commented Jun 10, 2023

Thanks for this feedback, it started as a hack idea I thought would be simple ("Just a simple field and an if") and I discovered how the paste file is structured (and how the adata fields order is important 😅) so I tried to stay very close of the existing, only extending what was needed.

I opted for unencrypted (clear on server's disk) but secured (cannot be altered without breaking the authenticity of the paste) récipient field because it was good enough for me and also simpler to implement, but I guess the field could be move to the encrypted part of the paste's data.

I also did not bother to update the test suite because I wasn't still sure how the hack would be accepted (but I took a look at it and it seemed doable).

You are right about the generic "recipient" field: Because it depends on third-party authentication it can be anything (user name, e-mail address, group name, etc.).

Depending on an external auth is the thing I was the more afraid of (for public acceptance of this hack) because it seemed a niche use-case (very few PrivateBin alternative have an user management system: I've tested some before opting for PrivateBin as a base for my hack).

In the end, the field can be seen as an "optional external authorizator": PrivateBin delegates access verification to an external system.

@r4sas
Copy link
Member

r4sas commented Jun 10, 2023

I think this is useless and also goes against the project's principle of "knowing" something about the stored data, the sender and the recipient (not considering cases where local law requires logging).

@rugk
Copy link
Member

rugk commented Aug 7, 2023

récipient field because it was good enough for me and also simpler to implement, but I guess the field could be move to the encrypted part of the paste's data.

I did not have a look into the code base, but from what I say: Hmm, how?

  • If you can check and verify the identity of the user in the Frontend it can always be modified and bypassed, so that is kinda insecure.
  • So having it backend-accessible (in the adata, yeah I like that too) sounds like the correct way IMHO. But also the backend then somehow needs to verify that. From what I see it seems to do that, though

I am not strictly opposed here, but yeah it alters one of the fundamental "principles" of PrivateBin (we never really documented that though?). But compared to all that Google Cloud Storage support etc. this change really looks soo small, so that may sound like a good idea. Also I could imagine this being helpful for requests like this: #1132

Code stuff

This is not supposed to be a code review, but some things:

  • can we generalize that somehow actually? I.e. it does not have to be the recipient, does it, who is set by the reverse proxy, it can be any secret string? So backend-vise this could be generalized.
  • docs needed, i agree
  • the verification is just done using ===, this could be a vulnerability, if you can somehow brute-force this. It's a kind of timing attack, so a constant-time comparison would appease me here haha. I know probably it can indeed not be misused, because the reverse-proxy sets it, but anyway…
  • if the header is not set for any reason, access is automatically granted. Hmm I don't like that, because let's design fail-safe resistant systems (randomly referring to this project that AFAIK may cause sth. like this) – if the config is set/this feature is enabled, access should not be granted IMHO. So if the reverse-proxy due to whatever (misconfiguration, outage of your auth provider etc.), may be broken and the header field is not there (e.g. renamed, typos etc.), PrivateBIn would still be safe.

@rugk rugk changed the title Add a recipient metadata/field? Add a recipient metadata/field for additional authentication via reverse-proxy Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants