Re: checking dependencies for known vulnerabilities

[Vlad Rozov]

Any other concerns regarding merging the PR? By looking at the active 
PRs on the apex core the entire conversation looks to be at the moot point.

Thank you,
Vlad

On 10/30/17 18:50, Vlad Rozov wrote:
On 10/30/17 17:30, Pramod Immaneni wrote:
On Sat, Oct 28, 2017 at 7:47 AM, Vlad Rozov <vro...@apache.org> wrote:

Don't we use unit test to make sure that PR does not break an existing
functionality? For that we use CI environment that we do not control 
and do
not introduce any changes to, but for example Apache infrastructure 
team
may decide to upgrade Jenkins and that may impact Apex builds. The same
applies to CVE. It is there to prevent dependencies with severe
vulnerabilities.

Infrastructure changes are quite different, IMO, from this proposal. 
While
they are not in our control, in majority of the cases, the changes 
maintain
compatibility so everything on top will continue to run the same. In 
this
case a CVE will always fail all PRs, the code changes have nothing to do
with introducing the CVE. I did make the exception that if a PR is 
bringing
in the CVE yes do fail it.
There were just two recent changes, one on Travis CI side and another 
on Jenkins side that caused builds for all PRs to fail and none of 
them was caused by code changes in any of open PRs, so I don't see how 
it is different.

A code change may or may not have relation to CVE introduced. For 
newly introduced dependencies, there may be known CVEs. In any case I 
don't think it is important to differentiate how CVE is introduced, it 
is important to eliminate dependencies with known CVEs.


There is no "stick" in a failed build or keeping PR open until 
dependency
issue is resolved or unit test failure is fixed. Unless an employer
punishes its employee for not delivering PR based on that vendor 
priority,
there is no "stick". As we already discussed, the community does not 
have a
deadline for a PR merge or for a release to go out. In a case there 
is a
problematic dependency (with CVE or category X license) you as a PMC
suppose to -1 a release (at least I will). Will you consider -1 as a
"stick"? For me, it is not about punishing an individual 
contributor, it is
a priority and focus shift for the entire community, not a "stick" 
for an
individual contributor.

The stick I am referring to is failing all PRs hoping that will make 
people
address CVEs. It's got nothing to do with an employer, people 
contributing
to open source can't expect they can control what the outcome will be or
what form it will take. You may be confusing this with some other 
issue. In
some of the arguments, you are assuming this scenario is similar to 
build
failures from failing unit tests and I am arguing that premise. I don't
think we should bring regular development to a halt whenever a 
matching CVE
is discovered, unless there is some other secondary reason like 
merging a
PR will make it difficult for a CVE fix to be made. Have you given a
thought to what I said about having a separate build that will notify 
about
CVEs.
As I mentioned, there is no stick, no deadlines and no issues keeping 
PRs open until builds can be verified on CI environment. Fixing a 
failed build is a priority for the *community* not a stick for an 
individual contributor.

I don't see why keeping PRs open (for whatever reason) brings regular 
development to a halt. Nobody is going to put github repo offline. 
Contributors may continue to open new PR, collaborate on existing PRs 
and add more changes (and need to be patient for those changes to be 
reviewed and accepted). The regular development will continue with the 
only exception that the next commit to be merged must address the 
build issue (whether it is a failed unit test or newly found CVE).

I don't see much value in a separate build and do not plan to put 
effort in that direction. Additionally, will not a separate build that 
only checks for CVE will trigger your initial concern of disclosing 
CVE in public?

Thank you,
Vlad


On 10/27/17 14:28, Pramod Immaneni wrote:

On Fri, Oct 27, 2017 at 11:51 AM, Vlad Rozov <vro...@apache.org> 
wrote:

On 10/26/17 11:46, Pramod Immaneni wrote:
On Thu, Oct 26, 2017 at 10:39 AM, Vlad Rozov <vro...@apache.org> 
wrote:
I guess you are mostly concerned regarding new CVE in an existing

dependency. Once such CVE is added to a database, will it be 
better to
know
about it or postpone discovery till we cut release candidate? In 
case
it
is
reported only during release cycle, there is much less time for the
community to take an action and it still needs to be taken (as a 
PMC
member
you are responsible for preventing release with severe security 
issue
going
out). If it is reported once the information becomes available, 
there
is
more time to research and either upgrade the dependency with newly
found
CVE, agree that it does not impact the project.

This would be the more commonly occurring scenario. We can 
always know

about the CVEs because of your changes. We don't need to fail 
builds to
do
that. I am not asking you to remove the reporting. There is no 
set time
for
a release so having less time during release for addressing relevant
CVEs
does not come up. There is also nothing preventing anyone from 
looking
at
these reports and taking action earlier.

I don't see why it will be more commonly occurring scenario, but 
I think
it is equally important to prevent new dependency with severe
vulnerabilities being introduced into the project and check existing
dependencies for newly discovered severe vulnerabilities.

How will we know about CVE if it is removed from CI build? Why 
require
manual verification when it can be done during CI build and does not
affect
builds done by individual contributors?

While there is no set time for a release, there is no set time for 
a PR
merge as well.

Yes, nothing prevents anyone from looking at the dependency
vulnerabilities, but there is a better chance that "anyone" does 
not mean
nobody if CI build fails.


I still do not understand why you value an individual contributor 
and PR

over the community and the project itself. Once there is a severe
security
vulnerability, it affects everyone who cares about the project,
including
all contributors. I don't see a problem with a PR being in a 
pending
(not
merged) open state till a build issue is resolved.

That is a mischaracterization that you have stated before as 
well. A

project cannot grow without contributions and without policies that
create
a supportive enviroment where that can happen, I don't see the 
need to
put
unnecessary obstacles for legitimate contributions that are not the
cause
of a problem. Everytime there is a matching CVE the PRs are going 
to get
blocked till that CVE is addressed and I am not confident we have 
the
bandwidth in the community to address this expediently. It is also
inaccurate to equate this to PR not being merged till build 
issues are
resolved as it derives from an assumption that CVE is same as a 
build
failure.

While project can't grow without individual contributions, 
project is a
result of a large number of contributions from a number of 
contributors.
Some of those contributors are not active anymore and will not 
provide
any
fixes should a vulnerability be found in their contribution. It 
becomes a
shared responsibility of all currently active community members 
and those
who submit PR are part of the community and share that 
responsibility,
are
not they? If a contributor considers him/herself as part of a 
community,
why he or she can't wait for the build issue to be resolved or better
take
an action on resolving the issue? The only possible explanation 
that I
see
is the one that I already mentioned on this thread.

If you see this as unnecessary obstacles for legitimate 
contributions,
why
to enforce code style, it is also unnecessary obstacle. Unit test? 
Should
it be considered to be optional for a PR to pass unit tests as 
well? What
if an environment change on CI side causes build to fail similar 
to what
happened recently? Should we disable CI builds too and rely on a
committer
or a release manager to run unit tests?  If CI build fails for 
whatever
reason, how can you be sure that if it fails for another PR as 
well, that
they both fail for the same reason and there is no any other 
reasons that
will cause a problem with a PR?

I don't know how failing PRs because of CVE, which we don't introduce,
don't control, no idea of and possibly unrelated would fall in the 
same
bucket as unit tests. I am at a loss of words for that. There is no 
reason
to block legitimate development for this. This can be handled 
separtely
and
in parallel. Maybe there is a way we can setup an independent job on a
build server that runs nightly, fails if there are new CVEs 
discovered and
sends an email out to the security or dev group. You could even 
reduce the
CVE threshold for this. I don't believe in a stick approach (carrot 
and
stick metaphor) and believe in proportional measures.



Thank you,
Vlad


On 10/26/17 09:42, Pramod Immaneni wrote:

On Thu, Oct 26, 2017 at 12:08 AM, Vlad Rozov <vro...@apache.org>
wrote:

There is a way to add an exception, but it needs to be 
discussed on a
case

by case basis. Note that CVEs are not published until a fix is
available.
For severity 8 CVEs I expect patches to become available for the
reported
version unless it is an obsolete version in which case, the 
upgrade
to
a
supported version is already overdue.

I think we should retain the ability to make that choice of 
what and
when

to upgrade rather than hard enforce it. Like I mentioned the 
CVE may
not
apply to us (it has happened before), even though it may be 
beneficial
upgrade generally when its not applicable, there may not be the
bandwidth
in community to do the necessary changes to upgrade to a newer 
version
especially if those dependencies don't follow semver (has happened
before
as well, remember effort with ning). My caution comes from
experiencing
this situation before.


I don't see how reporting helps. If a build succeeds, I don't 
expect

anyone to look into the report, it is only when CI build fails,
committers
and reviewers look into the details.

We can add a mandatory step during release that we need to assess
CVEs

matching this criteria before proceeding with the release. 
This could
end
up requiring upgrade of some dependencies and in other cases it 
may
not
be
needed. This assessment can also happen more often in adhoc 
fashion
offline
before release based upon interest from community. I am also 
open to
making
it mandatory with every PR, in future, like you are suggesting, 
if we
see
sufficient uptake in community on these issues. From experience 
this
is
not
there currently and hence I don't want to do this now.


IMO, it does not matter how CVE is introduced. It may be a new
dependency

with an existing CVE or it can be a new CVE for an existing
dependency.
In
both cases, dependency with the CVE needs to be fixed.

In one case the PR is not directly responsible for the issue and
hence

we
should avoid penalizing them or block them.



Thank you,

Vlad

On 10/25/17 11:58, Pramod Immaneni wrote:

Thanks that sounds mostly fine except what happens if there is 
a cve

matching that severity in a dependency but it doesnt affect us
because
let's say we don't exercise that part of functionality *and* 
there
isn't a
fix available or there is a fix but the upgrade requires 
significant
effort
(for example if we need to move to a new major version of the
dependency
or
something like that). Is there a way to add an exception like 
we did
for
checkstyle in the interim. How about reporting instead of 
failing
the
builds. One of the steps in release process could be to 
investigate
these
reports before proceeding with the release. If a PR 
introduces new
cves
by
virtue of adding a new dependency or changing an existing 
version,
that
would be of interest and can be subject to failure. Is there 
a way
to
distinguish that?

On Wed, Oct 25, 2017 at 8:52 AM Vlad Rozov <vro...@apache.org>
wrote:

A CVE (should there be a vulnerability in existing or a newly
introduced

dependency) will not be exposed during the CI build, but the 
build

will
fail if the CVE has severity 8 or above. To get the details, it
will
be
necessary to run dependency check manually.

Thank you,

Vlad

On 10/24/17 16:27, Pramod Immaneni wrote:

There was a lot of discussion on this but looks like there 
was no
final

agreement. Can you summarize what your PR does? Are we 
disclosing
the
actual vulnerabilities as part of the automated build for 
every
PR?
That
would be a no-no for me. If it is something that requires 
manual
steps,

for

example as part of a release build, that would be fine.
On Mon, Oct 23, 2017 at 1:16 PM, Vlad Rozov <vro...@apache.org>
wrote:

Please see https://github.com/apache/apex-core/pull/585 and
APEXCORE-790.
Thank you,

Vlad

On 9/14/17 09:35, Vlad Rozov wrote:

Do you expect anything else from the community to recognize a

contribution other than committing it to the code line? Once
there

is
a
steady flow of quality contributions, the community/PMC will
recognize

a

contributor by making that contributor a committer.
Thank you,

Vlad
On 9/12/17 13:05, Sanjay Pujare wrote:

For a vendor too, quality ought to be as important as 
security
so
I
don't

think we disagree on the cost benefit analysis. But I get 
your

drift.
By "creative incentive" I didn't imply any material incentive

(although a
gift card would be nice :-)) but more along the lines of 
what a

community
can do to recognize such contribution.
Sanjay

On Tue, Sep 12, 2017 at 8:10 AM, Vlad Rozov 
<vro...@apache.org>
wrote:

I guess we have a different view on the benefit and cost
definition.
For
me the benefit of fixing CI build, flaky unit test, severe
security
issue

is huge for the community and is possibly small (except for a
security
issues) for a vendor.
By "creative" I hope you don't mean that other community
members,

users
and customers send a contributor a gift cards to 
compensate for

the

cost
:). For me PR that is blocked on a failed CI build is
sufficiently

incentive for a contributor to look into why it fails and 
fixing
it.

Thank you,
Vlad
On 9/11/17 23:58, Sanjay Pujare wrote:

I don't want to speak for others and I don't want to
generalize.
But

an

obvious answer could be "cost-benefit analysis".
In any case we should come up with a creative way to
"incentivize"

members
to do these tasks.