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

Generate apps with SecTester #743

Merged
merged 25 commits into from
Apr 6, 2022
Merged

Generate apps with SecTester #743

merged 25 commits into from
Apr 6, 2022

Conversation

jwoertink
Copy link
Member

@jwoertink jwoertink commented Mar 19, 2022

Fixes #740

Right now this is in WIP to get things rolling, but a few questions still need answering...

Major parts left to handle:

  • - How do we test this? We can test that the code gets generated, but we now say you need a NEXPLOIT_TOKEN to exist, and it doesn't. Also, even if we did, this would end up connecting to Bright on each PR, and nightly run.
  • - Figure out how the API only specs will run. If you're not doing a browser app, you should still have the option to test that your API endpoints are good
  • - Need to update the generated github CI so it includes the -Dwith_sec_tests, but only if you've added that in.
  • - Figure out where to point users for API documentation. e.g. "Ok, I've added this in, my built-in tests run... now what? How do I add new ones?"

/cc. @bararchy

require "lucky_sec_tester"

# Signup for a `NEXTPLOT_TOKEN` at
# [NeuraLegion](https://app.neuralegion.com/signup)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The company name is now called Bright, but as of this comment, the Github org and this URL are still pointed to NeuraLegion... I don't want to confuse people, so this will need to change once the company's name is fully moved over.

src/generators/web.cr Outdated Show resolved Hide resolved
@bararchy
Copy link
Contributor

@jwoertink if we need to configure the API key for Lucky let me know and I can make sure the Lucky Org in Bright app has multiple engines to run all needed parallel scans :)

@grepsedawk
Copy link
Contributor

Is SecTester free for private source?

@bararchy
Copy link
Contributor

@grepsedawk yeha :) you can signup for an API key for free https://app.neuralegion.com/signup

@@ -17,7 +17,7 @@ describe "Initializing a new web project" do
File.delete("test-project/.env")
compile_and_run_specs_on_test_project
File.read("test-project/Procfile").should contain "test_project"
File.read(".github/workflows/ci.yml").should contain "postgres"
File.read("test-project/.github/workflows/ci.yml").should contain "postgres"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turns out this has been wrong for a while 😅

@bararchy
Copy link
Contributor

bararchy commented Apr 3, 2022

@jwoertink I saw you're using node12 which should be OK, I verified in the docs that it's supported: https://docs.brightsec.com/docs/installation-options#npm

So anything 10-14 should be good, I also looked for the touch thingy but I can't find it. One reason for process to not be up is something in the ENV? I'll try and add more logs. can you check locally with the API key and with LOG_LEVEL=DEUBG? see that at least locally we get it working

@jwoertink
Copy link
Member Author

That touch happens on the LuckySecTester shard itself Since that shard doesn't actually need to run any nexploit checks.

https://github.com/luckyframework/lucky_sec_tester/blob/afc4d99ee1b8fbd371a2b9b0a5f0ac1bee185859/.github/workflows/ci.yml#L49-L50

I can use a newer version of node, but it just seems like the scanner.client isn't being called or something. I'll try locally and see what I can find out.

@bararchy
Copy link
Contributor

bararchy commented Apr 3, 2022

@jwoertink can we try and re-run using latest luckyframework/lucky_sec_tester@346ceb3 ?

… we turn down to medium threshold. Also pussing a full URL to the Target for the javascript checks
scan_with_cleanup do |scanner|
api_headers = HTTP::Headers{"Content-Type" => "application/json", "Accept" => "application/json"}
target = scanner.build_target(Api::SignIns::Create, headers: api_headers) do |t|
t.body = {"user" => {"email" => "aa%40aa.com", "password" => "123456789"}}.to_json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwoertink as this is a JSON API call, do we actually need to encode the @ here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not....

@jwoertink jwoertink marked this pull request as ready for review April 4, 2022 18:20
@jwoertink
Copy link
Member Author

it's green!!!

Comment on lines -68 to -70
- uses: actions/setup-node@v3.1.0
with:
node-version: "12.x"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use the node that comes installed on ubuntu-latest

Comment on lines -4 to -6
push:
branches: [main]
pull_request:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was causing this to run on every single commit. It should be a separate PR, but I needed it to quiet down with all the testing.

Comment on lines 41 to 43
- uses: actions/setup-node@v3.1.0
with:
node-version: "12.x"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually needs to go away

@@ -0,0 +1,23 @@
require "../spec_helper"

{% if env("RUN_SEC_TESTER_SPECS") == "1" %}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone that wants to run this spec locally would need a Bright API key... That's not really going to happen, so we can treat this like the Heroku specs. As long as this spec passes in CI, and the rest pass locally, we should be fine

@bararchy
Copy link
Contributor

bararchy commented Apr 4, 2022

Amazing work @jwoertink! 🟢 light!

@@ -22,7 +22,7 @@ print_done
<%- end -%>

notice "Installing shards"
shards install | indent
shards install --ignore-crystal-version | indent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a "hey i need this to work for now" contribution which should be removed? Or should this be kept in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a tough one... Without it, all tests again < 1.2.2 fail because of https://github.com/luckyframework/lucky_sec_tester/blob/346ceb3567110e433863dd1997afcec6243db3e5/shard.yml#L7 That's an easy one to drop down, but then we can only drop down to 1.1.1 https://github.com/NeuraLegion/sec_tester/blob/9db160150b4cc15ebb6d6722a25ee55a5c5af3ce/shard.yml#L7 and CLI tests against 1.0.0

Do I just raise the min test to 1.1.1 for CLI since 1.4 comes out tomorrow? Then I could update the other shard, and remove this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I updated the LuckySecTester to be >=1.1.1. It would still fail due to checking 1.0.0. So I still have to either use the flag, or drop 1.0.0 support 🤔 I'm sort of ok with going with the latter. I feel like once 1.4.0 drops, few people will be wanting to upgrade Lucky and continue using 1.0.0....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any reason not to have this flag in there, I was just curious. It's complicated, and I get that.

Copy link
Contributor

@robacarp robacarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine. The way this pattern scales makes me nervous, but I get that we need to have a few dice in the bucket before we can have enough to roll yahtzee.

@jwoertink jwoertink merged commit 88c10fc into main Apr 6, 2022
@jwoertink jwoertink deleted the issues/740 branch April 6, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding option for integrated sec tester
5 participants