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

[WIP] Simple LDAP sync #396

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
5 participants
@ViViDboarder
Copy link
Contributor

commented Feb 15, 2019

So this is definitely a work in progress as it's quite messy. This is also my first time using Rust, so this is probably not very idiomatic and contains mistakes.

This patch adds several new config values for setting up a connection to an LDAP server, then polling it periodically and generating invites for users that do not yet have accounts. In short, I roughly did what was discussed in #40 and used what @mprasil wrote in #173 to add users.

A couple of notes:

  1. There are still a bunch of print statements that I ought to clean up
  2. I'm not sure if the way that I kicked off the thread is the best way to do so. Tokio only allows one Reactor Core, so if something is added later on, this may cause an issue. It may be possible to initialize a core in the main function and then pass handles down to the other functions.
  3. There are a few error handling items within the Futures that I can't quite figure out. Not sure how best to deal with them.

Please let me know any feedback. I'm happy to iterate on this!

@dani-garcia

This comment has been minimized.

Copy link
Owner

commented Feb 15, 2019

This is nice work, but the last time this was discussed (in #122), I heard of the directory connector, which is the tool used by the official server to run the synchronization. I think that would be a better way to implement this, that way we avoid having to manage the sync threads and we also get support for other kinds of directories. Also, we'll need to handle users being deleted from the directory, which we'll need to delete in our server (or probably just disable, to be safe). What do you think?

@mprasil

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

Correct me if I'm wrong, but I think this serves a slightly different purpose than the directory connector. Directory connector has to run outside of the server as it also manages the organization users according to the LDAP, which requires access to the Organization secrets.

This thing essentially just invites users to the server based on LDAP membership and lets the users create organizations and groups themselves.

So I don't think these two things are equal.

Having that said, I'm not entirely sure if this really should be part of the bitwarden_rs. To me this could be an external service, that would call admin API (using the token) and invite users as needed. Generally speaking maybe it would be better to maintain API on our side and manage this as separate project - we could then support more of these "connectors".

It looks like most of the code is written, so you'd just have to pull it out of bitwarden_rs sources. This also avoids the struggle with Tokio as you'd have that fully in your control.

I do think however, that this is useful thing to have - especially if you want to use bitwarden_rs in a company.

@evaryont

This comment has been minimized.

Copy link

commented Feb 20, 2019

So I'm using the directory connector between AD and hosted Bitwarden at work, and we have put it into production.

  • The directory connector needs RO access to your LDAP, but administrative access to your Bitwarden Organization via credentials for a preexisting user
  • It will automatically create new users in Bitwarden and invite them to your organization. They will have to finish the account creation process as if they signed up themselves
  • Users are in the "invited" state, you still have to confirm their invitation.
  • It can optionally delete users, if you configure it to do so. Equivalency is based on email address
  • It does not sync the account's password. If the user chooses to keep their LDAP and BW passwords in sync, that's done by them, manually
  • It does this entirely via the organizations import and related API endpoints
  • Users and groups in Bitwarden but not recognized in the directory are left alone. (There a little state managed by the directory connector to track which users/groups were created by it)
  • Collections aren't touched at all

My recommendation would be to implement the import endpoint in bitwarden_rs and let people use the directory connector (or alternative implementations) independently.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

All great feedback! Thanks!

I agree, that seems quite simpler and more robust. I guess I should have done some more research before jumping in.

At least I got to learn Rust so I can take on the next patch!

@mprasil

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2019

@ViViDboarder I still think you might be closer to move this to external tool and use the admin API already in place than you'd be to implement API required for the connector to work. (If I understand it right, connector also requires groups support, that we don't have implemented at all) So the code isn't really wasted if you decide to move that way.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Got it. That makes sense.

@evaryont

This comment has been minimized.

Copy link

commented Feb 21, 2019

If I understand it right, connector also requires groups support, that we don't have implemented at all

Not necessarily. The directory connector can sync users and groups, but you can disable either or. (Which funnily enough means you can do all the configuration for it to sync...nothing. Or just groups, but no users.) Of course, this is just one tool to push stuff via the import endpoint; @ViViDboarder's alternative could support only users.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

That makes sense.

I actually originally built this as a standalone so that I could learn rust without the dependencies. I should be able to easily extract it. Seems like the invite user api already exists, so there won't be anything to do in this repo.

Shouldn't be terribly hard to learn how to make HTTP requests and then I can publish something.

I'm also a little busy lately, so it may take a few weeks until I get to it though.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

I guess for sake of clarity, I'm planning to implement an auto invite system (much as here) which will invite any users in LDAP that don't exist.

The simplest form (and first patch) will not touch bitwarden_rs and will be a small service that will read from LDAP and then post to invite_user for each individual user. This will result in a large number of failed requests for users that are already invited.

Future enhancement would be something like the org system where all users get sent in a single request and then the server can decide who to invite and who not to invite. Alternatively, could implement a list users and list invites endpoint that will allow the LDAP service to send an invite to net new users.

I'm open to either of those as possible solutions.

@mprasil

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

@ViViDboarder there's also an admin API to get current users, so you could easily filter out users already present on the server and only invite missing ones. With reqwest it should be fairly easy to do. We use reqwest crate here as well to download icons if you need some inspiration, althought I think the crate is dead simple to use.

@dani-garcia

This comment has been minimized.

Copy link
Owner

commented Feb 26, 2019

At the moment, the admin API doesn't allow listing the current users, you'll have to parse the HTML, but if it's useful I can add it anyway.

The current invite endpoint in the admin API already errors when the user exists so it shouldn't be a problem, and it also has a delete user endpoint, which could also be used.

That said, I think the better alternative long term would be to support the endpoint /api/organizations/<id>/import, even if we don't implement all functionality. It already receives a list of users, that way there is no need to make a ton of requests.

This would require the LDAP sync client to get an authentication token though, which involves hashing the password and all that work that the official clients do, which might complicate things a little bit.

@mprasil

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

Ah my bad, I didn't realize the list of users is applied to the html template server side. Thanks for correction.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

The delete endpoint only deletes users, not invites.

Anyway, without a list endpoint, can’t really do deletion.

That’s something I didn’t consider with the auth token. How long is it good for? Would the user have to store their credentials in this service so it can renew? That doesn’t seem ideal. Maybe instead a shared secret could be used.

@dani-garcia

This comment has been minimized.

Copy link
Owner

commented Feb 26, 2019

Depends on what token you are talking about:

  • The login token, that is used for the normal bitwarden operations expires after two hours, but there is a refresh token can be used to get a valid new login token.

  • For the admin page, the auth token expires after twenty minutes, and it requires logging in again with the ADMIN_TOKEN.

On the first case, you can get away storing only the refresh token and generating a new auth token when needed, but to get one you need to generate the correct master password hash.
On the second case, you need to store the actual ADMIN_TOKEN, but the procedure is simpler.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Oh, got it. That's much simpler. Thanks.

@Zuko

This comment has been minimized.

Copy link

commented Mar 26, 2019

Any progress?

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Good question. The answer is no. I've been caught up with some personal things, but they are winding down now. I'll see if I can get something out this week/weekend.

@Zuko

This comment has been minimized.

Copy link

commented Mar 26, 2019

OK, thanks for answer ;)

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Ok. LDAP querying complete in a standalone crate.

Next step is to call the API to set up.

Progress can be followed here: https://github.com/ViViDboarder/bitwarden_rs_ldap

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Hey @dani-garcia, I'm having a hard time understanding how the admin API works.

I can see the endpoint that I need to call here:

#[post("/invite", data = "<data>")]

But I'm not sure how I pass it an Admin Token for authentication.

Also, I see a get_users function here:

fn get_users(_token: AdminToken, conn: DbConn) -> JsonResult {

Would that not return JSON to to look up existing users? Or is this module not what I think it is?

@dani-garcia

This comment has been minimized.

Copy link
Owner

commented Mar 29, 2019

That's pretty old code, currently we don't have a get_users function in the admin page, but we can add it back if you need it.

To login, you'll need to make a POST request to <domain>/admin/, the contents of that post are just a single token field using an application/x-www-form-urlencoded form. So something like token=mytokenabc.

This should return a 200 status code and set a cookie header in the response, something like this:

Set-Cookie: BWRS_ADMIN=<long_jwt_string>; Path=/admin; Max-Age=1200

You'll need to save that long string somewhere and use it to authenticate the admin. Note that they expire in 20 minutes, so you'll need to renew them frequently.

Then, to make any request, you'll need to set the cookie header to that value, like:

Cookie: BWRS_ADMIN=<long_jwt_string>;

To invite a user, you'll have to call the POST <domain>/admin/invite with the cookie set and a JSON body like:

{
  "email": "user_to_invite@domain.com"
}
@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Perfect. Thanks! Got it working in curl, so shouldn't be long now.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Ok. Latest version is working.

Need to add some documentation, better error handling, and some command line args to make it a bit easier to work with.

Right now it doesn't look for existing users or delete. It's just attempting to invite all users. It's redundant and probably inefficient if you have a large org. Will try to add a get users endpoint later and then try to use that.

@mprasil

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

I guess it goes without saying, but I feel like deleting users automatically should be an optional feature that one has to turn on. I've seen too many configuration mistakes resulting in LDAP reporting zero users and that sync would be quite painful.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Totally. If I get around to implementing it, I’d make it optional for sure.

Thoughts on publishing this? Should I keep it in its own repo with its own Docker image? I can provide a Compose file demonstrating how to use the two together.

Also for configuration, I see a library called envy for deserializing structs from environment variables, but not sure how to merge those values with the struct from the toml file. Think that’s worth looking into?

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Ok. Just pushed new PRs #455 and #456 to allow retrieval of both existing users and invites. The former is merged, the later not yet.

I've pushed all updates to my other project, https://github.com/ViViDboarder/bitwarden_rs_ldap, such that it now uses these endpoints to be a little more efficient.

Next step is to publish a Docker image.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@mprasil could you rebuild the official Docker image so that I can test my compose example?

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Merged and kicked off a Docker Hub build for the LDAP sync service. Also updated the Readme to explain usage.

This I should replace this branch with a Readme update pointing to the LDAP service?

@mprasil

This comment has been minimized.

Copy link
Collaborator

commented Apr 13, 2019

The build is now triggered, you should have image ready in about an hour. There's also ritwardenrs/dev:master that was built automatically and is already available for testing.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Thanks @mprasil. I verified that everything works as expected.

Should I push an update to readme or just wiki? If wiki, we can close this.

@mprasil

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

I think this definitely belongs to wiki.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.