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

Hackweek security scanning #1289

Merged
merged 4 commits into from Jul 18, 2017

Conversation

Projects
None yet
3 participants
@vitoravelino
Member

vitoravelino commented May 19, 2017

Opening this PR to start getting some feedback. As @mssola mentioned to me, this PR became quite large. Thinking about that I'm gonna work on the data tables filtering in another branch.

There are still some missing tasks that I'll be updating as soon as I finish them.

  • Convert .js with templates to .vue
  • Move repository to the top of the page
  • Move pull/push/role info somewhere else
  • Restyle fav icon
  • Restyle delete repository
  • Delete tag appears once a tag is selected
  • Tests

Extracted from @mssola comments:

There are some issues when the security scanning is not available:

  • No tags are returned, when it should return at least the tags with no vulnerability information.
  • A flashy message "Unable to fetch newer tags data" is shown for every polling. This is distracting, and we should show it once instead.
  • The tags#show panel should have a "No vulnerabilities found" instead of having an empty space.

Bits missing:

@vitoravelino vitoravelino requested a review from mssola May 19, 2017

@vitoravelino

This comment has been minimized.

Show comment
Hide comment
@vitoravelino

vitoravelino May 19, 2017

Member

@mssola I've managed to solve the issues with the undefined errors by just making poltergeist driver use the phantomjs executable handled by the phantomjs gem that was installed by I guess not being used so far.

Another portusctl test failed after you've fixed the flags security. This new one is in man_spec.rb.

Looking forward to your review. Thanks! 😃

Member

vitoravelino commented May 19, 2017

@mssola I've managed to solve the issues with the undefined errors by just making poltergeist driver use the phantomjs executable handled by the phantomjs gem that was installed by I guess not being used so far.

Another portusctl test failed after you've fixed the flags security. This new one is in man_spec.rb.

Looking forward to your review. Thanks! 😃

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola May 22, 2017

Contributor

Reviewing the functionality itself, not the code:

  • I'd like to preserve the colors of the star widget. That blue color fit with the general design of Portus better.
  • If I only have one tag, I can read: "Showing from 1 to 1 of 1 entries", which is quite redundant. I'd say that if there's only one page, then stating "Showing 1 entry" should be enough, or even remove this message altogether in this case.
  • If there's no security backends enabled, then this feature should be entirely hidden.
Contributor

mssola commented May 22, 2017

Reviewing the functionality itself, not the code:

  • I'd like to preserve the colors of the star widget. That blue color fit with the general design of Portus better.
  • If I only have one tag, I can read: "Showing from 1 to 1 of 1 entries", which is quite redundant. I'd say that if there's only one page, then stating "Showing 1 entry" should be enough, or even remove this message altogether in this case.
  • If there's no security backends enabled, then this feature should be entirely hidden.
@vitoravelino

This comment has been minimized.

Show comment
Hide comment
@vitoravelino

vitoravelino May 22, 2017

Member

If there's no security backends enabled, then this feature should be entirely hidden.

By enabled you mean any of the 3 security options being != "", correct? Is there already a helper for that or should I write one?

Member

vitoravelino commented May 22, 2017

If there's no security backends enabled, then this feature should be entirely hidden.

By enabled you mean any of the 3 security options being != "", correct? Is there already a helper for that or should I write one?

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola May 22, 2017

Contributor

By enabled you mean any of the 3 security options being != "", correct? Is there already a helper for that or should I write one?

Yeah exactly. There's no helper for that, but I can write it. Let me do it.

Contributor

mssola commented May 22, 2017

By enabled you mean any of the 3 security options being != "", correct? Is there already a helper for that or should I write one?

Yeah exactly. There's no helper for that, but I can write it. Let me do it.

@vitoravelino

This comment has been minimized.

Show comment
Hide comment
@vitoravelino

vitoravelino May 22, 2017

Member

@mssola requested changes were done.

Also, I've written more tests to hit the expected code coverage but there still 2 classes (security/zypper|clair.rb) and 1 piece of code (application_controller.rb:39) that I couldn't write any test because I have no knowledge about it. 😕

Member

vitoravelino commented May 22, 2017

@mssola requested changes were done.

Also, I've written more tests to hit the expected code coverage but there still 2 classes (security/zypper|clair.rb) and 1 piece of code (application_controller.rb:39) that I couldn't write any test because I have no knowledge about it. 😕

@jzelinskie

This comment has been minimized.

Show comment
Hide comment
@jzelinskie

jzelinskie May 24, 2017

Howdy,

I'm a maintainer over at Clair. It'd be awesome if you guys added yourself to our list of documented integrations: https://github.com/coreos/clair/blob/master/Documentation/integrations.md

If you have any questions about integrating with Clair lmk. I'm always on #clair on freenode IRC.

jzelinskie commented May 24, 2017

Howdy,

I'm a maintainer over at Clair. It'd be awesome if you guys added yourself to our list of documented integrations: https://github.com/coreos/clair/blob/master/Documentation/integrations.md

If you have any questions about integrating with Clair lmk. I'm always on #clair on freenode IRC.

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola May 25, 2017

Contributor

I'm a maintainer over at Clair. It'd be awesome if you guys added yourself to our list of documented integrations: https://github.com/coreos/clair/blob/master/Documentation/integrations.md

Sure thing. Once we get this merged I'll submit a PR. Thanks!

Contributor

mssola commented May 25, 2017

I'm a maintainer over at Clair. It'd be awesome if you guys added yourself to our list of documented integrations: https://github.com/coreos/clair/blob/master/Documentation/integrations.md

Sure thing. Once we get this merged I'll submit a PR. Thanks!

@vitoravelino

This comment has been minimized.

Show comment
Hide comment
@vitoravelino

vitoravelino May 31, 2017

Member

Passing! Woohoo!

Member

vitoravelino commented May 31, 2017

Passing! Woohoo!

@mssola mssola referenced this pull request Jun 10, 2017

Merged

REST API #1299

andrew2net added a commit to Vad1mo/Portus that referenced this pull request Jun 23, 2017

@mssola

LGTM. I'll proceed doing some manual tests as soon as possible. Sorry for the delay.

const TEXT_ALERT_ELEMENT = '#float-alert p';
const HIDE_TIMEOUT = 5000;
function scheduleHide() {

This comment has been minimized.

@mssola

mssola Jul 12, 2017

Contributor

I don't think this is needed... As in, I'd prefer to inline this code where it's used.

@mssola

mssola Jul 12, 2017

Contributor

I don't think this is needed... As in, I'd prefer to inline this code where it's used.

This comment has been minimized.

@vitoravelino

vitoravelino Jul 12, 2017

Member

What do you mean with "inline this code where it's used"? Can you give an example?

@vitoravelino

vitoravelino Jul 12, 2017

Member

What do you mean with "inline this code where it's used"? Can you give an example?

This comment has been minimized.

@mssola

mssola Jul 12, 2017

Contributor

You don't have to create a new function for something simple that it's only used once.

@mssola

mssola Jul 12, 2017

Contributor

You don't have to create a new function for something simple that it's only used once.

This comment has been minimized.

@vitoravelino

vitoravelino Jul 12, 2017

Member

Ohhh... Gotcha! Will change that then. Thanks!

@vitoravelino

vitoravelino Jul 12, 2017

Member

Ohhh... Gotcha! Will change that then. Thanks!

@@ -0,0 +1,3 @@
import Vue from 'vue';
export default new Vue({});

This comment has been minimized.

@mssola

mssola Jul 12, 2017

Contributor

What is this ?

@mssola

mssola Jul 12, 2017

Contributor

What is this ?

This comment has been minimized.

@vitoravelino

vitoravelino Jul 12, 2017

Member

It returns a plain new Vue object with built-in methods like $emit, $on, $off. It's the same thing as doing $({}) in jQuery like I tried to do here.

I did this to simulate a pub/sub object and let different components communicate without compromising everything with coupling. As an example, normally the child/inner components shouldn't handle data/services requests as a good practice. It should bubble an event and let the page/root parent component handle all of this. This way if we need to check how data is being handled at first, we will know where to take a look at.

Not sure if I explained well enough for you. Let me know. :)

@vitoravelino

vitoravelino Jul 12, 2017

Member

It returns a plain new Vue object with built-in methods like $emit, $on, $off. It's the same thing as doing $({}) in jQuery like I tried to do here.

I did this to simulate a pub/sub object and let different components communicate without compromising everything with coupling. As an example, normally the child/inner components shouldn't handle data/services requests as a good practice. It should bubble an event and let the page/root parent component handle all of this. This way if we need to check how data is being handled at first, we will know where to take a look at.

Not sure if I explained well enough for you. Let me know. :)

This comment has been minimized.

@mssola

mssola Jul 12, 2017

Contributor

Thanks for the explanation 👍

@mssola

mssola Jul 12, 2017

Contributor

Thanks for the explanation 👍

@vitoravelino

This comment has been minimized.

Show comment
Hide comment
@vitoravelino

vitoravelino Jul 12, 2017

Member

No worries about the delay, we had more important stuff going on. :)

Member

vitoravelino commented Jul 12, 2017

No worries about the delay, we had more important stuff going on. :)

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 14, 2017

Contributor

@vitoravelino I've manually tested this and it looks good. Just fix my comment and we'll be good to proceed.

Contributor

mssola commented Jul 14, 2017

@vitoravelino I've manually tested this and it looks good. Just fix my comment and we'll be good to proceed.

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 14, 2017

Contributor

Bits missing:

- [ ] Ping @jordimassaguerpla and/or @MaximilianMeister, so they can review the updated gems.
- [ ] Add documentation.
- [ ] Add documentation to coreos/clair about this integration (see this page).
- [ ] Rebase into proper commits.

Update: pasted into the main comment...

Contributor

mssola commented Jul 14, 2017

Bits missing:

- [ ] Ping @jordimassaguerpla and/or @MaximilianMeister, so they can review the updated gems.
- [ ] Add documentation.
- [ ] Add documentation to coreos/clair about this integration (see this page).
- [ ] Rebase into proper commits.

Update: pasted into the main comment...

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 14, 2017

Contributor

There are some issues when the security scanning is not available:

- [ ] No tags are returned, when it should return at least the tags with no vulnerability information.
- [ ] A flashy message "Unable to fetch newer tags data" is shown for every polling. This is distracting, and we should show it once instead.
- [ ] The tags#show panel should have a "No vulnerabilities found" instead of having an empty space.

Update: pasted into the main comment...

Contributor

mssola commented Jul 14, 2017

There are some issues when the security scanning is not available:

- [ ] No tags are returned, when it should return at least the tags with no vulnerability information.
- [ ] A flashy message "Unable to fetch newer tags data" is shown for every polling. This is distracting, and we should show it once instead.
- [ ] The tags#show panel should have a "No vulnerabilities found" instead of having an empty space.

Update: pasted into the main comment...

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 17, 2017

Contributor

We have to rebase on top of master because this conflicts with the merged #1319.

Contributor

mssola commented Jul 17, 2017

We have to rebase on top of master because this conflicts with the merged #1319.

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 17, 2017

Contributor

Rebased

Contributor

mssola commented Jul 17, 2017

Rebased

mssola added a commit that referenced this pull request Jul 17, 2017

Added documentation for security scanning
See #1289

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>

mssola added a commit that referenced this pull request Jul 18, 2017

Added documentation for security scanning
See #1289

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 18, 2017

Contributor

@vitoravelino I've rebased everything into four commits. Hopefully I haven't broken anything ... Anyways, could you add your sign-off in all the commits ?

Contributor

mssola commented Jul 18, 2017

@vitoravelino I've rebased everything into four commits. Hopefully I haven't broken anything ... Anyways, could you add your sign-off in all the commits ?

mssola and others added some commits Apr 4, 2017

Basic support for security scanning
Scanners supported: CoreOS Clair, zypper-docker (experimental, based on
an unmerged branch). I've also included a `dummy` scanner, for
development purposes.

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Vítor Avelino <vavelino@suse.com>
Added an authentication layer for the API
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Vítor Avelino <vavelino@suse.com>
Introducing Vue.js for the frontend
With it we introduce some changes on the UI of the repositories#show
page, and we have added the tags#show page.

Moreover, the frontend has been greatly modularized, to modern
standards.

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Vítor Avelino <vavelino@suse.com>
Fixed issues introduced by previous commits
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Vítor Avelino <vavelino@suse.com>
@vitoravelino

This comment has been minimized.

Show comment
Hide comment
@vitoravelino

vitoravelino Jul 18, 2017

Member

@vitoravelino I've rebased everything into four commits. Hopefully I haven't broken anything ... Anyways, could you add your sign-off in all the commits ?

Done, @mssola.

Member

vitoravelino commented Jul 18, 2017

@vitoravelino I've rebased everything into four commits. Hopefully I haven't broken anything ... Anyways, could you add your sign-off in all the commits ?

Done, @mssola.

@mssola mssola merged commit 3c360bf into master Jul 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mssola mssola deleted the hackweek_security_scanning branch Jul 18, 2017

mssola added a commit to mssola/clair that referenced this pull request Jul 18, 2017

Added Portus integration
Since SUSE/Portus#1289 got merged, Portus now integrates security
scanners in order to fetch vulnerabilities for the images stored in the
on-premise Docker registry. CoreOS Clair is a supported backend, so you
can now use Clair for this. This is all explained in the documentation:

  http://port.us.org/features/6_security_scanning.html

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>

@mssola mssola referenced this pull request Jul 18, 2017

Merged

Added Portus integration #433

Vad1mo added a commit to Vad1mo/Portus that referenced this pull request Jul 18, 2017

vitoravelino added a commit to vitoravelino/Portus that referenced this pull request Jul 19, 2017

Added latest missing changes from security scanning PR (#1289)
- Not showing the flashy message "Unable to fetch newer tags data" for every polling.
- Showing "No vulnerabilities found" on tags#show panel istead of an empty space.

vitoravelino added a commit to vitoravelino/Portus that referenced this pull request Jul 19, 2017

Added latest missing changes from security scanning PR (#1289)
- Not showing the flashy message "Unable to fetch newer tags data" for every polling.
- Showing "No vulnerabilities found" on tags#show panel istead of an empty space.

mssola added a commit that referenced this pull request Jul 19, 2017

Merge pull request #1328 from vitoravelino/missing-changes-security-s…
…canning

Added latest missing changes from security scanning PR (#1289)

mssola added a commit to mssola/Portus that referenced this pull request Jul 20, 2017

Added latest missing changes from security scanning PR (#1289)
- Not showing the flashy message "Unable to fetch newer tags data" for every polling.
- Showing "No vulnerabilities found" on tags#show panel istead of an empty space.

@mssola mssola referenced this pull request Aug 2, 2017

Closed

Sidebar links don't work. #1265

andrew2net added a commit to Vad1mo/Portus that referenced this pull request Aug 5, 2017

andrew2net added a commit to Vad1mo/Portus that referenced this pull request Aug 9, 2017

andrew2net added a commit to Vad1mo/Portus that referenced this pull request Aug 9, 2017

andrew2net added a commit to Vad1mo/Portus that referenced this pull request Aug 9, 2017

KeyboardNerd pushed a commit to KeyboardNerd/clair that referenced this pull request Feb 2, 2018

Added Portus integration
Since SUSE/Portus#1289 got merged, Portus now integrates security
scanners in order to fetch vulnerabilities for the images stored in the
on-premise Docker registry. CoreOS Clair is a supported backend, so you
can now use Clair for this. This is all explained in the documentation:

  http://port.us.org/features/6_security_scanning.html

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment