Please see PR https://github.com/apache/apex-core/pull/585 comments and
APEXCORE-790 for the further discussion and the resolution. There is
still one open item - updating contributor/committer guidelines if
someone wants to help with it.
Thank you,
Vlad
On 11/2/17 16:55, Vlad Rozov wrote:
Can you check if there is a possibility to distinguish between
existing dependencies and dependencies introduced in a PR. AFAIK, it
does not exist.
Thank you,
Vlad
On 11/2/17 16:50, Pramod Immaneni wrote:
Check is fine as long as we can identify whether the CVE was
introduced by
a PR. It might still be useful to fix a CVE even if there is no release,
downstream projects might be able to use it.
On Thu, Nov 2, 2017 at 4:42 PM, Thomas Weise <t...@apache.org> wrote:
Check as part of PR build is needed to ensure no new issues are
introduced.
I don't think it is necessary to have another build to notice
pre-existing
issues since releases won't occur without prior PRs.
Whitelisting suggestion is for pre-existing problems and not for those
introduced by a PR.
On Thu, Nov 2, 2017 at 11:35 PM, Pramod Immaneni
<pra...@datatorrent.com>
wrote:
If the PR introduces a CVE by all means lets fail it initally and
later
look at whitelisting it if needed. Why fail all PRs that haven't
caused
the
problem. What happens when there are no PRs in a given period,
wouldn't
it
be better to know above crtiical CVEs (that this proposal is trying to
address) sooner than wait till some random PR is raised. What if there
are
no PRs for a month. I think it makes sense to have a separate build
that
will catch these errors.
On Thu, Nov 2, 2017 at 3:17 PM, Ananth G <ananthg.a...@gmail.com>
wrote:
Only the first PR should be broken if cve with a higher score than
agreed
is introduced .Once you whitelist subsequent builds will not fail?
- The dependency check plugin is enabled to fail/break the build if a
violation is detected
- A PR introducing a risk will either “acknowledge” the cve by
creating
the whitelist entry as part of the process or will break the build
- The reviewer/s would notice either the changes to the whitelist
file
or
the build break
- The review process would agree upon the approach and ensure JIRA
ticket
creation or question why an alternative cannot be used
- whitelist would be maintained in a suppressions file example given
here
https://jeremylong.github.io/DependencyCheck/general/suppression.html
- subsequent PR update should no longer break the build
- there is a list of JIRA items for cve which needs to be
addressed in
the
subsequent releases as well as a set of artefacts in the project
modules
that summarise the community’s awareness of the issue.
Regards
Ananth
On 3 Nov 2017, at 8:38 am, Pramod Immaneni <pra...@datatorrent.com>
wrote:
The question is which build? We can definitely make it a mandatory
release
step and also have nightly builds that are meant for just these,
detection
of CVEs and even possible infra changes that might break tests.
Those
will
work even if there are no PRs.
On Thu, Nov 2, 2017 at 1:21 PM, Ananth G <ananthg.a...@gmail.com>
wrote:
My vote would be to break the build. This can then force a
“whitelisting”
configuration in the maven plugin to be created as part of the
review
process ( along with a new JIRA ticket ).
The concern would then be to ensure that the community works
towards a
resolution of the JIRA. Not breaking the build for me is tech debt
slipping
without anyones notice. Fixing the JIRA is a hygiene process which
I
believe cannot take a back burner as compared contributor welfare
and
would
need commitments from the contributor ( and/or others).
On a related note, looking at apache spark, there seems to be CVE
listings
which the spark community has taken care of as the releases
progressed.
http://www.cvedetails.com/vulnerability-list/vendor_id-
45/product_id-38954/Apache-Spark.html <http://www.cvedetails.com/
vulnerability-list/vendor_id-45/product_id-38954/Apache-Spark.html>
.
Regards,
Ananth
On 3 Nov 2017, at 4:48 am, Sanjay Pujare <san...@datatorrent.com>
wrote:
I like this suggestion. Blocking the PR is too drastic. I also
second
Pramod's point (made elsewhere) that we should try to encourage
contribution instead of discouraging it by resorting to drastic
measures.
If you institute drastic measures to achieve a desired effect
(e.g.
getting
contributors to look into CVEs and build infrastructure issues) it
can
have
the opposite effect of contributors losing interest.
Sanjay
On Wed, Nov 1, 2017 at 1:25 PM, Thomas Weise <t...@apache.org>
wrote:
Considering typical behavior, unless the CI build fails, very few
will
be
interested fixing the issues.
Perhaps if after a CI failure the issue can be identified as
pre-existing,
we can whitelist and create a JIRA that must be addressed
prior to
the
next
release?
On Wed, Nov 1, 2017 at 7:51 PM, Pramod Immaneni <
pra...@datatorrent.com
wrote:
I would like to hear what others think.. at this point I am
-1 on
merging
the change as is that would fail all PR builds when a matching
CVE
is
discovered regardless of whether the PR was the cause of the CVE
or
not.
On Wed, Nov 1, 2017 at 12:07 PM, Vlad Rozov <vro...@apache.org>
wrote:
On 11/1/17 11:39, Pramod Immaneni wrote:
On Wed, Nov 1, 2017 at 11:36 AM, Vlad Rozov <vro...@apache.org
wrote:
There is no independent build and the check is still necessary
to
prevent
new dependencies with CVE being introduced.
There isn't one today but one could be added. What kind of
effort
is
needed.
After it is added, we can discuss whether it will make sense to
move
the
check to the newly created build. Even if one is added, the
check
needs
to
be present in the CI builds that verify PR, so it is in the
right
place
already, IMO.
Look at Malhar 3.8.0 thread. There are libraries from Category
X
introduced as a dependency, so now instead of dealing with
the
issue
when
such dependencies were introduced, somebody else needs to
deal
with
removing/fixing those dependencies.
Those were directly introduced in PRs. I am not against
adding
additional
checks that verify the PR better.
Right and it would be much better to catch the problem at the
time
it
was
introduced, but Category X list (as well as known CVE) is not
static.
Thank you,
Vlad
On 11/1/17 11:21, Pramod Immaneni wrote:
My original concern still remains. I think what you have is
valuable
but
would prefer that it be activated in an independent build
that
notifies
the
interested parties.
On Wed, Nov 1, 2017 at 11:13 AM, Vlad Rozov <
vro...@apache.org
wrote:
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.
