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

Enable admin to send test OSSEC alert via the admin interface #2771

Merged
merged 3 commits into from Jan 4, 2018

Conversation

redshiftzero
Copy link
Contributor

Status

Ready for review

Description of Changes

Implements #2770, enabling the sending of a test alert upon request to admins:

admin-test-ossec

Testing

New tests should pass (and you can verify journalist-error.log is monitored in app OSSEC config)

If you have prod VMs, you can make build-debs, vagrant provision /prod/, and verify the alert is produced as expected following the documentation added in the PR

Deployment

No special considerations

Checklist

If you made changes to the app code:

  • Unit and functional tests pass on the development VM

If you made changes to documentation:

  • Doc linting passed locally

@ghost
Copy link

ghost commented Dec 30, 2017

I'm concerned this may not be in the right place. Should an admin journalist know about OSSEC at all ?

@redshiftzero
Copy link
Contributor Author

Hmm, true - let me move this to the System configuration page added in #2769, that's a better home for this feature

@ghost
Copy link

ghost commented Jan 4, 2018

let me move this to the System configuration

Sorry to be a kill joy but ... what I wonder is if we should start exposing things the journalist (even one who is an admin trusted to handle users) should never have to worry about. The OSSEC alerts are for the admin to see and the journalist should never know about it. I'm insisting because I have a feeling that if we open this door it will be very difficult to close when and if we change our mind later.

That being said if you weighted the pros/cons already, I fully support your decision :-)

@redshiftzero
Copy link
Contributor Author

Yep, I totally agree that we should not show this to a journalist user, and only to admins. In my view the purpose of the Admin Interface should be to provide a user-friendly place for an admin to do any app server tasks that would otherwise be done via securedrop-admin or SSHing into the server (both are obviously not particularly user friendly). So the question here is: do we have SecureDrop journalists that have "admin" accounts on the webapp i.e. they don't want to or need to know what OSSEC alerts (or other system config) information is? I'm not aware of users of this type, but if we do find that there is a need for this third role on the application - a middle-ground user that can add/remove users but not to do any other administrative tasks, then I think the right thing to do would be to create this additional user type.

Happy to discuss further, it's an important point :)

@kushaldas
Copy link
Contributor

That also means we will have to maintain access details between 3 types of users, that will surely increase the chances of mistakes. Maybe in code, maybe in actual operations. I would love to have an option without making users and their access states more complex.

@ghost
Copy link

ghost commented Jan 4, 2018

do we have SecureDrop journalists that have "admin" accounts on the webapp ... I'm not aware of users of this type

That was the information I did not have, thanks for clarifying. I was assuming journalists wiht admin permissions. In reality, as you observed in the field, the same person who uses the the journalist interface with the admin permission is also the the sysadmin that maintains the servers.

I'll approve the pull request and let you merge if my understanding is correct. If I misinterpreted your explanation, please correct me :-)

@redshiftzero redshiftzero merged commit 17cdc93 into develop Jan 4, 2018
@redshiftzero redshiftzero deleted the admin-test-ossec-button branch January 4, 2018 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants