DAST feature is currently relying on one unique tool called ZAProxy. Like other security features before, this tool is wrapped in a single "top-level" project: https://gitlab.com/gitlab-org/security-products/dast.
Though, while we've migrated all tools to dedicated analyzers, we haven't done it yet for dast (nor for license-management which has its own issue).
This creates inconsistencies that block stage wide initiatives in UX and engineering.
Intended users
Further details
Even if we have a single tool to provide dast today, we might replace it or add new ones someday. We also want to support 3rd party integrations so moving to an architecture that is made for it will make things simpler for everyone.
add instructions in the current release job for current dast project to create docker image tag aliases and push additionality under https://gitlab.com/gitlab-org/security-products/analyzers/dast
update the vendored templates and documentation to point to the new image
deprecate the usage of docker images under https://gitlab.com/gitlab-org/security-products/dast in documentation
@sethgitlab@derekferguson here is the issue about the discussion we had in the Secure Stage Weekly, please raise your concern on the final goal as if we disagree on it, there is no point in doing the first step in %13.0, which is primarily there to unlock !28617 (merged)
@gonzoyumo@plafoucriere My main issue with this is the name change. The location change doesn't really bother me. But, as @sethgitlab mentioned in the weekly meeting, I don't really see the point in changing the name of the image, since we don't want to be tied specifically to ZAP. There will likely be other analyzers in DAST in the near future. I understand the transparency that this is trying to provide but it will cause issues for us if we add other scanning engines to the DAST image. Also, we've added functionality on top of ZAP and will continue to build out that functionality as we move forward. If DAST does not change its name, but does change the location, how would that affect the existing MR?
I understand your point @derekferguson, but I think there's a confusion here: the point is not to have a single/unique DAST analyzer that will be tight forever to Zap. If you plan to change the underlying tool one day, it should be a new analyzer then, based on the chosen new tool. This change will be transparent for the users, as the new analyzers will be used directly in the DAST template. This is what we have followed so far in SAST, Dependency Scanning, and Container Scanning without issues.
For example, Container Scanning relies on Klar only today, and used to be something very generic: https://gitlab.com/gitlab-org/security-products/container-scanning. If we move away from Klar in the future, it won't be a problem for us. It's even more clear for users who were customizing the analyzer job. By introducing a new job, their parameters and configuration will still work during the transition (deprecation process).
I hope that helps.
For example, Container Scanning relies on Klar only today, and used to be something very generic: https://gitlab.com/gitlab-org/security-products/container-scanning. If we move away from Klar in the future, it won't be a problem for us. It's even more clear for users who were customizing the analyzer job.
One thing we still miss for klar, is job naming, which should be klar-container_scanning instead of container-scanning, to follow the convention currently in place for SAST and DS. Though this is not a blocker and has other implications (e.g. usage ping tracking), so I did not put it there.
I don't really see the point in changing the name of the image, since we don't want to be tied specifically to ZAP. There will likely be other analyzers in DAST in the near future. I understand the transparency that this is trying to provide but it will cause issues for us if we add other scanning engines to the DAST image
The current dast project is actually already tied to ZAP, as it can't work without it.
If the dast project currently also contains abstract work that could also apply to any other DAST tool you'd like to add, then that's fine and you'll probably need to put that in a separate module when you'd want to introduce another analyzer to reuse that instead of duplicating, exactly like we did for other analyzers and the common library. And it's already being discussed to possibly have intermediary layer like a SAST specific shared module that would increase capabilities of SAST only analyzers, without leaking into analyzers from other features that don't need it.
If instead, you plan on putting all the tools in the same project and maybe the same docker image, I think this is probably the wrong direction, since this is what we're getting away from for all other tools. And maybe DAST doesn't have the same split points as other features (e.g. languages), but I'll definitely recommend not putting all into the same image. To me, this is where DAST engineers' knowledge may impact the decision and show us if something outweighs the benefits of the current approach we've well tested for other features, and of course, if it outweighs the beauty of consistency .
I think that we disagree on how we want users to configure and use the analyzers. I understand the transparency for the analyzers and being able to enable/disable them individually for things like SAST, where you are looking at code and need to distinguish between languages that should be scanned. For DAST, having that level of configuration causes more work and confusion, since they just want to scan a website or API (or both). They don't need the ability to configure different analyzers specifically and I wouldn't want them to be able to. This adds unnecessary complexity to the configuration workflow. I have no problem telling users what analyzers we are using in our documentation, but there's no reason to split them into multiple images. DAST is fundamentally different from the scans that have insight into the code and needing to install multiple images just to kick off a single scan against a website doesn't make sense. It puts more burden on the user than I'd like for no real reason.
I think that the main difference in my mind is that if we add another analyzer, it will be complementary to ZAP and necessary for each DAST scan, rather than users having an option to use one or the other analyzer. I'm also not as concerned with the way things are right now, with us only using ZAP, as I am with looking towards the future and not locking us into a direction that will make it more difficult to deliver a unified product to our users.
I think that we disagree on how we want users to configure and use the analyzers. I understand the transparency for the analyzers and being able to enable/disable them individually for things like SAST, where you are looking at code and need to distinguish between languages that should be scanned. For DAST, having that level of configuration causes more work and confusion, since they just want to scan a website or API (or both).
I think this means you may prevent your users from directly configuring the tools if they are experts in that domain, or you'll have to handle a lot of options on your own. This statement needs to be confirmed in the context of DAST, as you probably won't have the same issues having DinD to orchestrate multiple tools for instance, but this is something we encountered with SAST and DS. Also, having separate jobs makes it easier to configure them independently but doesn't mean you enforce that, you still can have common settings that apply to all your analyzers, as we do for SAST and DS too.
Another concern is that by putting that logic all into the same dast project/image/job, you might encounter issues with 3rd parties not playing well with our built-in solution. By designing our architecture as we did for analyzers, we build for that kind of extension seamlessly. For instance, if the dast project holds the logic to dedupe what comes from multiple scanners, you might have duplicates with 3rd parties reports as you'll miss that logic on the rails side (this is a made-up example). Again, this is what we've experienced so far and I understand it might be hard to predict how the DAST feature will handle multiple scanners if the underlying goal is more about improving or enriching results than getting more results from different sources.
@gonzoyumo Those are all great points for us to keep in mind. @sethgitlab and I talked about a couple of them today and we don't believe that we'll run into the same problem with requiring DinD for the DAST image.
Also, I'm not as concerned with expert users of these tools. I'm not sure how it is with the other groups, but with DAST, this typically has not been a dev or devops task. It mostly falls to the security teams to verify security post-deployment of their production application. By trying to integrate it into the development/devops life cycle, my experience with our users is that we will run into more people that have never set up a DAST scan than we will users who are experts. That being said, we provide a way for experts to pass in their own configs for ZAP (to a point) and I think that we could expand that to any analyzer we might integrate. Regardless, I think that tailoring the DAST config experience (especially in the UI) to a lower level of knowledge and experience will increase usage for DAST
As far as 3rd party integrations, I'll have to do some more thinking about that. I agree that having that logic in the image could cause some issues for anyone wanting to integrate another DAST tool. It's one reason why I've been pushing grouping vulnerabilities differently on the dashboard and allowing users to break them out into their own issue if they think that they are caused by different parts of the code. But, I realize that this is a very DAST-centric way of viewing the vulnerabilities and that for other groups, each vuln is almost certainly caused by different parts of code. For DAST, it's rarely that clear-cut and I've never seen a DAST tool that lists each vulnerability found on each individual page as a separate item. For DAST, it makes sense to have each vulnerability have a 'location' property (URL for DAST) and have the dashboard show them all as one "vulnerability" with multiple locations cited. The user can then make the decision for themselves about whether a specific location needs a separate issue created than the others. But, again, this runs counter to what makes sense for the other analyzers. But, now I'm getting into other issues that aren't necessarily directly related to this.
For the record, I have created a zaproxy project in the analyzers namespace. Nothing definitive, we can always change the name, but I needed something right now to avoid failing pipelines. Don't panic :)
Also note that if we do this change, we might also want to start deprecating DAST_VERSION to start using a variable that would be tight to zaproxy instead.
Well, we are pretty sure that we will have another analyzer in the next 3-6 months, not to replace ZAP, but to add on functionality. Also, since we are taking ownership of the python scripts that we wrap ZAP in, it makes more sense to me to have a 'DAST_VERSION', rather than the ZAP versioning on the image. Since we will likely be changing things in the image that aren't related to the ZAP version, we should keep our own versioning.
@cam_swords@sethgitlab I assume that the python scripts are included in that image and it isn't just a standalone ZAP image, is that correct? My point may be incorrect if I'm wrong in that assumption.
What @plafoucriere means here is not to set the ZAP tool's version, but this var purpose is to set the version of the ZAP analyzer, which is our own project that wraps the ZAP tool.
Similarly, other features will probably move from a single SAST_VERSION to a per analyzer env var, so we can set them independently. Sounds not necessary yet though, and still under discussions for other features.
I think that I understand, but by making it a variable that is tied to the zaproxy version, as @plafoucriere suggests, it limits our ability to make major changes to the image that are not tied to the ZAP version. For example, we include a lot of ZAP add-ons in the image. It is completely feasible that we could change an add-on that would introduce some major new functionality, but the underlying ZAP version would stay the same. I understand the reasoning behind exposing the ZAP version, but I don't like having an image version that is tied to ZAP. In the end, it's my opinion that our image is more than just the ZAP base analyzer and I'm very much opposed to tying it to ZAP. I'm not trying to be opaque or remove any transparency at all. In fact, I feel like it is the opposite. Tying the image closely to ZAP in name and version could create a false transparency and cause customer issues when things may not work exactly like the standalone analyzer would if they downloaded it for themselves. By documenting the ZAP version and the versions of the plugins we are using, that gives the transparency. But, it also leaves room for us to be transparent about things we are adding on top of the analyzer to provide additional value.
Why are we worried about where the projects are located and structured, when what we are most concerned about is where the docker images are published?
Because we've coupled these two issues, its made this issue more contentious and harder to move forward on. If the issue gets limited to just the location of where images are published, then it much easier to move forward.
Wouldn't a more clear solution be to have all security images in a project like:
Technically, that would work, of course, we can push wherever we want as soon as we configure the right tokens. But so far we had the source code in the same project so I think it's convenient for users if they want to collaborate.
The other problem being that we can't easily move images, at least not with a breaking change. That's why we'd like to "only" move dast and license-compliance instead of all of them.
On the other hand, this could help with isolating our images for #18984 (closed).
AFAIR that's what @plafoucriere initially wanted to achieve with the bundle idea. And this probably stays a valid idea as we have non-analyzer data to expose, like external databases we currently ask customers to fetch on their own. But maybe there are other routes too.
That being said, this option won't address other organizational issues we have by not following the same structure for our projects/repositories.
By putting all analyzers in the same group, it's easier for team members to understand our projects, it's easier to mutualize effort on similar problems like development and release scripts, QA and basically all similar needs we can have.
Having a dedicated project to wrap each tool is making it easier to separate concerns and individually manage their development cycle. This might become even more true with the increase or community contributions. Otherwise, it means going the monorepo direction, which AFAIR is not what best suites with GitLab (this is changing though).
we are pretty sure that we will have another analyzer in the next 3-6 months, not to replace ZAP, but to add on functionality.
This means you'll face that situation during the year and will have to decide between:
breaking dast into 3 parts: a common module, ZAP specific analyzer, XXX specific analyzer
including all tools in dast, orchestrating and aggregating reports from there
Option 2 is what we started with for SAST and DS, and what we're getting rid of to move to Option 1.
After discussing with @sethgitlab it looks like there could be good reasons for DAST to stay with Option 2, and this is to me what needs to be evaluated here.
I agree that there are good reasons for DAST to stay with Option 2. My reasoning is customer-focused and does not take into account any technical or engineering organizational information, so it's a bit one-sided. To me, breaking it into 3 parts will require a user to have all 3 images just to be able to run a single, simple DAST scan. Unlike SAST and DS, where you will specifically want to include only the images that apply to your project, DAST is all or nothing. The only logical break point in DAST (in general) is between site and api tests. But, even there, for us, ZAP does both, so it doesn't makes sense for us to make that distinction. To me, delivering a more cohesive DAST image for customer ease of use overrides consistency between the different groups. The goal of SAST, DAST, and DS is the same, but the process by which it finds the vulnerabilities is fundamentally different. I feel like the packaging of the different features should reflect the way that a user would use it, rather than just considering consistency between the groups in Secure.
It's not about having 3 images, just 2 (1 per analyzer, the shared module is just for building the others).
Anyway, I get your point, and indeed if you think there is no value in running separate analyzers concurrently, then having 2 images does not make sense. Thanks for explaining further!
There seems to be a lot of future speculation in these threads.
If we think the DAST landscape is going to change significantly in the next 6 months, then I vote we make the most minimal change now, so that in 6 months we can make a decision based on information we are much more certain about.
It's a major part of my job to speculate about the future. But, I agree. That's really why I am not comfortable making the name change for the image at this point. I don't have a strong opinion about the location change, but I do about the name change. If the location change is necessary for the other groups, I am ok with that. But, I don't believe that keeping the dast image name the same will cause any issues cross-stage, other than engineering consistency.
@derekferguson I 100% agree. Defining the final name was only to avoid another renaming later this year but you all have shown reasonable doubts to follow the existing convention. All the immediate concerns and later ones are addressed by putting it under analyzers/dast.
Thanks a lot for all the feedback, I think we've learned more about DAST world
@plafoucriere I've updated the proposal of this issue to reflect the new location.
@gonzoyumo@plafoucriere I apologize for my overly verbose responses just now on previous comments. I didn't see these new comments. Thank you for the discussion, it's been great and has made me think through some things that I hadn't yet.
This proposal sounds fine to me. I'm not opposed to it. Unless @sethgitlab has any objections, I'm good to move forward with it.
@plafoucriere To get this moving, are you okay with us just updating our publishing scripts to push to the analyzers/dast project container registry for now?
@plafoucriere do you need someone from the DAST team to update the release script to publish to the analyzers/dast container registry, or was this something you were planning on doing yourself?
As this is not urgent, I guess we can ask someone from the DAST team, but I'm also fine doing it.
We don't need to do it in %13.0, so I'll let @sethgitlab assign someone accordingly. I can review if you want to make the change of course. Thanks!
@plafoucriere To migrate this project, we will need to clone the project in the new project. Once we do that we will have an 'old project'.
In the old project is there anyway to set the repo to readonly mode. I can set it to only project members, but that still does not prevent someone from accidentally submitting code to the old project. I can't find any gitlab setting for setting a repo to readonly.
@plafoucriere This is the other approach I came up with to effectively set the repo to readonly.
Settings > General> Visibility, project features. * Disable Merge Requests Settings > Repository > Protected Branches *master * Set "Allowed to merge" to "No one" * Set "Allowed to Push" to No one