ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Denis Garus <garus....@gmail.com>
Subject Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Date Thu, 17 Oct 2019 13:21:38 GMT
Hello, Pavel!

Thank you for the feedback!

I've created IEP-38 that describes the Ignite Sandbox [1].

Yes, the issue requires documentation (there is the flag "Docs equired"),
but common practice is to write documentation in the end.

>> 1) Why do you run resource injection through security and how it tested?
>> 2) Why do you check security at *dumpThreads* and *wrapThreadLoader
*methods?
>>      These methods are needed only for internal node processes.
>> 4) There are suspicious security checks at:
>> CacheOperationContext:37
>> GridCacheDefaultAffinityKeyMapper:86
>> PageMemoryImpl:874
>> I'm not following why they are needed.

These questions have a common answer.
A user-defined code can call any operation through the public API of
Ignite. But he may don't have permissions to execute this operation
successfully.
For example, to put a value into a cache, it requires permissions for
accessing to reflection API and reading system property
IGNITE_ALLOW_ATOMIC_OPS_IN_TX.
In that case, we have to use AccessController#doPrirvelged call to exclude
a user-defined code from checking of permissions.
SecurityUtils#doPriveleged does calling AccessController#doPrirvelged a
more convenient way.

>> 3) Have you tested security if a compute job is canceled?
You are right; we should add a test for the cancel case.
But, for now, we have the issue [2] with the current SecurityContext for
the canceling of ComputeJob.

1. https://cwiki.apache.org/confluence/display/IGNITE/IEP-38%3A+Sandbox
2. https://issues.apache.org/jira/browse/IGNITE-12300

пн, 14 окт. 2019 г. в 16:16, Pavel Kovalenko <jokserfn@gmail.com>:

> Denis,
>
> The idea of having a sandbox for running a user-defined code is useful, but
> I don't fully understand the implementation approach.
> There is no detailed description of the ticket about what public API
> methods or configuration parameters should be covered.
> There is no description of what have done in initial PR and how.
> First of all, there should be an umbrella ticket that should contain all
> public API points and configuration parameters where used defined code may
> be run.
> Without a full list of all possible user-defined code injections, we can't
> track what was covered and where are a possible security lacks.
> I've checked PR and I have the following questions:
> 1) Why do you run resource injection through security and how it tested?
> 2) Why do you check security at *dumpThreads* and *wrapThreadLoader
> *methods?
> These methods are needed only for internal node processes.
> 3) Have you tested security if a compute job is canceled?
> 4) There are suspicious security checks at:
> CacheOperationContext:37
> GridCacheDefaultAffinityKeyMapper:86
> PageMemoryImpl:874
> I'm not following why they are needed.
>
>
>
>
> пн, 14 окт. 2019 г. в 12:19, Anton Vinogradov <av@apache.org>:
>
> > Fully agree with the benchmark's importance.
> > Currently, we're not able to perform proper benchmarking.
> > Slava, Is it possible to ask you to check the solution using GridGain's
> > benchmarking environment?
> >
> > On Mon, Oct 14, 2019 at 12:07 PM Вячеслав Коптилин <
> > slava.koptilin@gmail.com>
> > wrote:
> >
> > > Hello Anton,
> > >
> > > > We should avoid heavy merges if possible.
> > > Why it should be avoided? To be honest, I don't see any reason for
> that.
> > > Every pull request can be and should be reviewed when it is done and
> > ready
> > > to be merged into the epic branch (IEP branch).
> > > So, the final review of the entire IEP is just a technical/trivial
> task,
> > in
> > > my opinion.
> > >
> > > If I am not mistaken, we are at the stage of preparing a new release
> > (2.8),
> > > right?
> > > And we are trying to add a new feature that may impact the performance.
> > > For example, affinity function, which can be overridden by the
> end-user,
> > > and therefore should be covered by `sandbox`.
> > > On the other hand, affinity function is a crucial component that is
> used
> > > very often.
> > > Are we really sure that the proposed change does not affect the
> > > performance? Do we have a benchmark?
> > >
> > > Please don't get me wrong, guys. I am not against the feature itself.
> > > Moreover, it is a great feature and improvement of security.
> > > I just want to say that we need to be sure that we are on the right way
> > of
> > > implementing this without affecting other developers.
> > >
> > > PS: This is just my opinion, which may be wrong.
> > >
> > > Thanks,
> > > S.
> > >
> > > пн, 14 окт. 2019 г. в 09:09, Anton Vinogradov <av@apache.org>:
> > >
> > > > Folks,
> > > >
> > > > We should avoid heavy merges if possible.
> > > > I'm ok with IEP to keep tasks properly, but strictly against
> all-in-one
> > > > "+27000,-18200" merges.
> > > > This task implements Sandbox (API + core) which covered by tests and
> by
> > > > some integrations with existing components, which is enough to merge.
> > > > The most important thing here is that we will be able to speed-up
> > Sandbox
> > > > coverage development once its core menged to the master.
> > > >
> > > > On Fri, Oct 11, 2019 at 5:41 PM Вячеслав Коптилин <
> > > > slava.koptilin@gmail.com>
> > > > wrote:
> > > >
> > > > > Hi Denis,
> > > > >
> > > > > In my humble opinion, the security (the sandbox feature is about
> > > > security,
> > > > > right?) either covers all APIs/subsystems or not.
> > > > > Security should work always and everywhere otherwise it is not
> > security
> > > > :)
> > > > >
> > > > > > From my point, we should divide the sandbox and features that
use
> > it.
> > > > > > Also, I added in the main features of Ignite (cache and compute)
> > the
> > > > > sandbox calls.
> > > > > And at this point, you mixed both in the same pull-request.
> > > > >
> > > > > > I don't see any problem to have the sandbox in the master branch
> > and
> > > > > implement covering for existing and new features if needed.
> > > > > On the other hand, this approach leads to ...
> > > > > ignite-123 [IEP-X] introduces new cool API
> > > > > ignite-124 [IEP-X] improved cool API
> > > > > ignite-125 [IEP-X] fixed a bug
> > > > > ignite-126 [IEP-X] fixed performance drop
> > > > > ignite-127 [IEP-X] Cache API uses new API
> > > > > ignite-127 [IEP-X] Compute grid uses new API
> > > > > ...
> > > > >
> > > > > Why should it be a part of the master branch history? All these
> > things
> > > > can
> > > > > be done on the feature branch, I think. Anyway, it is up to you.
> > > > >
> > > > > Thanks,
> > > > > S.
> > > > >
> > > > > пт, 11 окт. 2019 г. в 16:31, Denis Garus <garus.d.g@gmail.com>:
> > > > >
> > > > > > From my point, we should divide the sandbox and features that
use
> > it.
> > > > > > The sandbox is fully implemented and has needed tests.
> > > > > >
> > > > > > Also, I added in the main features of Ignite (cache and compute)
> > the
> > > > > > sandbox calls.
> > > > > >
> > > > > > I don't see any problem to have the sandbox in the master branch
> > > > > > and implement covering for existing and new features if needed.
> > > > > >
> > > > > > пт, 11 окт. 2019 г. в 15:21, Вячеслав Коптилин
<
> > > > slava.koptilin@gmail.com
> > > > > >:
> > > > > >
> > > > > > > Hi Denis,
> > > > > > >
> > > > > > > Yep, I understand the scope of the ticket, but... I think
it is
> > > not a
> > > > > > good
> > > > > > > idea to merge partly implemented feature(s) into the master
> > branch.
> > > > > > > Especially, at this moment. We are at the stage of preparing
a
> > new
> > > > > > release
> > > > > > > and I doubt that all improvements, tests (unit tests,
> integration
> > > > > tests,
> > > > > > > and performance tests) can be implemented before the release
> > branch
> > > > is
> > > > > > cut
> > > > > > > off.
> > > > > > > Personally, I would prefer to create an epic/feature branch
for
> > > these
> > > > > > > activities. In that case, we can implement a feature step
by
> step
> > > and
> > > > > > merge
> > > > > > > it into the master branch once all components are covered.
> > > > > > >
> > > > > > > > But, sure, we should execute any user-defined code
in the
> > sandbox
> > > > on
> > > > > a
> > > > > > > remote node. Feel free to create issues.
> > > > > > > will do.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > S.
> > > > > > >
> > > > > > > пт, 11 окт. 2019 г. в 14:52, Denis Garus <garus.d.g@gmail.com
> >:
> > > > > > >
> > > > > > > > Hello, Slava!
> > > > > > > >
> > > > > > > > The scope of the issue is limited by the following
features:
> > > > > > > >
> > > > > > > >    - StreamReceiver for DataStreamer;
> > > > > > > >    - EntryProcessor;
> > > > > > > >    - ComputeJob;
> > > > > > > >    - filter and transformer for ScanQuery.
> > > > > > > >
> > > > > > > > But, sure, we should execute any user-defined code
in the
> > sandbox
> > > > on
> > > > > a
> > > > > > > > remote node.
> > > > > > > > Feel free to create issues.
> > > > > > > >
> > > > > > > > Thanks for the feedback!
> > > > > > > >
> > > > > > > > пт, 11 окт. 2019 г. в 13:26, Вячеслав
Коптилин <
> > > > > > slava.koptilin@gmail.com
> > > > > > > >:
> > > > > > > >
> > > > > > > > > Hello Denis, Anton,
> > > > > > > > >
> > > > > > > > > Could you please clarify the following aspect?
Do we need
> the
> > > > same
> > > > > > > > > changes/capabilities related to Continuous Queries,
Disco
> > > > > listeners,
> > > > > > > > > CacheStore Factories etc?
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > S.
> > > > > > > > >
> > > > > > > > > пт, 11 окт. 2019 г. в 12:24, Anton Vinogradov
<
> av@apache.org
> > >:
> > > > > > > > >
> > > > > > > > > > Folks,
> > > > > > > > > >
> > > > > > > > > > As a prereviewer, I'd like to say that the
solution looks
> > > good
> > > > to
> > > > > > me,
> > > > > > > > but
> > > > > > > > > > fresh eyes would be good.
> > > > > > > > > >
> > > > > > > > > > On Fri, Oct 11, 2019 at 9:40 AM Denis Garus
<
> > > > garus.d.g@gmail.com
> > > > > >
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hello, Igniters!
> > > > > > > > > > >
> > > > > > > > > > > I've raised the PR [1] with the sandbox
for AI [2].
> > > > > > > > > > > Could somebody review it?
> > > > > > > > > > >
> > > > > > > > > > > If you have questions and prefer the
Slack, I've
> created
> > > the
> > > > > > > channel
> > > > > > > > > [3].
> > > > > > > > > > >
> > > > > > > > > > > 1. https://github.com/apache/ignite/pull/6707
> > > > > > > > > > > 2. https://issues.apache.org/jira/browse/IGNITE-11410
> > > > > > > > > > > 3. https://app.slack.com/client/T4S1WH2J3/CP8JER880
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message