Ivan, thank you very much for the review and proposals! I've made more descriptive JavaDoc; take a look, please.
> Please think about driving creating IEP on security API overhaul prior to 2.9. Yes, I'll create IEP on security API soon. I also have some ideas about how we can improve Ignite's security. Thank you! пт, 27 мар. 2020 г. в 11:41, Ivan Rakov <ivan.glu...@gmail.com>: > Denis, > > In general, code changes look good to me. If we decide to keep security API > in its current state for a while, I highly recommend to extend its > documentation. We don't have descriptive javadocs or articles about > security API so far, so I expect that next contributors will face > difficulties in untangling security logic. Let's help them a bit. > See more details in my JIRA comment: > https://issues.apache.org/jira/browse/IGNITE-12759 > > On Thu, Mar 26, 2020 at 5:54 PM Ivan Rakov <ivan.glu...@gmail.com> wrote: > > > Denis, > > > > I'll review your PR. If this issue is a subject to be included in 2.8.1 > in > > emergency mode, I'm ok with the current API changes. > > Please think about driving creating IEP on security API overhaul prior to > > 2.9. I believe that you are the most suitable Ignite community member to > > drive this activity. I'd love to share some ideas as well. > > > > On Tue, Mar 24, 2020 at 2:04 PM Denis Garus <garus....@gmail.com> wrote: > > > >> Hi, guys! > >> > >> > >> I agree that we should rework the security API, but it can take a long > >> time. > >> > >> And currently, our users have certain impediments that are blockers for > >> their job. > >> > >> I think we have to fix bugs that IEP-41 [1] contains as soon as possible > >> to > >> support our users. > >> > >> From my point of view, IEP-41 is the best place to track bug fixing. > >> > >> > >> > >> 1. > >> > >> > https://cwiki.apache.org/confluence/display/IGNITE/IEP-41%3A+Security+Context+of+thin+client+on+remote+nodes > >> > >> > >> вт, 24 мар. 2020 г. в 12:26, Ivan Rakov <ivan.glu...@gmail.com>: > >> > >> > Alexey, > >> > > >> > That can be another version of our plan. If everyone agrees that > >> > SecurityContext and SecuritySubject should be merged, such fix of thin > >> > clients' issue will bring us closer to the final solution. > >> > Denis, what do you think? > >> > > >> > On Tue, Mar 24, 2020 at 10:38 AM Alexei Scherbakov < > >> > alexey.scherbak...@gmail.com> wrote: > >> > > >> > > Why can't we start gradually changing security API right now ? > >> > > I see no point in delaying with. > >> > > All changes will go to next 2.9 release anyway. > >> > > > >> > > My proposal: > >> > > 1. Get rid of security context. Doing this will bring security API > to > >> > more > >> > > or less consistent state. > >> > > 2. Remove IEP-41 because it's no longer needed because of change [1] > >> > > 3. Propose an IEP to make security API avoid using internals. > >> > > > >> > > > >> > > > >> > > пн, 23 мар. 2020 г. в 19:53, Denis Garus <garus....@gmail.com>: > >> > > > >> > > > Hello, Alexei, Ivan! > >> > > > > >> > > > >> Seems like security API is indeed a bit over-engineered > >> > > > > >> > > > Nobody has doubt we should do a reworking of > GridSecurityProcessor. > >> > > > But this point is outside of scope thin client's problem that we > are > >> > > > solving. > >> > > > I think we can create new IEP that will accumulate all ideas of > >> > Ignite's > >> > > > security improvements. > >> > > > > >> > > > >> Presence of the separate #securityContext(UUID) highlights that > >> user > >> > > > indeed should care > >> > > > >> about propagation of thin clients' contexts between the cluster > >> > nodes. > >> > > > > >> > > > I agree with Ivan. I've implemented both variants, > >> > > > and I like one with #securityContext(UUID) more. > >> > > > > >> > > > Could you please take a look at PR [1] for the issue [2]? > >> > > > > >> > > > 1. https://github.com/apache/ignite/pull/7523 > >> > > > 2. https://issues.apache.org/jira/browse/IGNITE-12759 > >> > > > > >> > > > пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <ivan.glu...@gmail.com>: > >> > > > > >> > > > > Alex, Denis, > >> > > > > > >> > > > > Seems like security API is indeed a bit over-engineered. > >> > > > > > >> > > > > Let's get rid of SecurityContext and use SecuritySubject > instead. > >> > > > > > SecurityContext is just a POJO wrapper over > >> > > > > > SecuritySubject's > >> > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > >> > > > > > It's functionality can be easily moved to SecuritySubject. > >> > > > > > >> > > > > I totally agree. Both subject and context are implemented by > >> plugin > >> > > > > provider, and I don't see any reason to keep both abstractions, > >> > > > especially > >> > > > > if we are going to get rid of transferring subject in node > >> attributes > >> > > > > (argument that subject is more lightweight won't work anymore). > >> > > > > > >> > > > > Also, there's kind of mess in node authentication logic. There > >> are at > >> > > > least > >> > > > > two components responsible for it: DiscoverySpiNodeAuthenticator > >> > (which > >> > > > is > >> > > > > forcibly set by GridDiscoveryManager, but in fact public) and > >> > > > > GridSecurityProcessor (which performs actual node auth logic, > but > >> > > > private). > >> > > > > I also don't understand why we need both > >> > > > > #authenticate(AuthenticationContext) and > >> > #authenticateNode(ClusterNode, > >> > > > > SecurityCredentials) methods while it's possible to set explicit > >> > > > > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this > is > >> > > > arguable; > >> > > > > perhaps there are strong reasons). > >> > > > > > >> > > > > Finally, areas of responsibility between IgniteSecurity and > >> > > > > GridSecurityProcessor are kind of mixed. As far as I understand, > >> the > >> > > > first > >> > > > > is responsible for Ignite-internal management of security logic > >> > > (keeping > >> > > > > thread-local context, caching security contexts, etc; we don't > >> expect > >> > > > > IgniteSecurity to be replaced by plugin provider) and the latter > >> is > >> > > > > responsible for user-custom authentication / authorization > logic. > >> To > >> > be > >> > > > > honest, it took plenty of time to figure this out for me. > >> > > > > > >> > > > > From my point of view, we should make GridSecurityProcessor > >> interface > >> > > > > public, rename it (it requires plenty of time to find the > >> difference > >> > > from > >> > > > > IgniteSecurity), make its API as simple and non-duplicating as > >> > possible > >> > > > and > >> > > > > clarify its area of responsibility (e.g. should it be > responsible > >> for > >> > > > > propagation of successfully authenticated subject among all > nodes > >> or > >> > > > not?) > >> > > > > to make it easy to embed custom security logic in Ignite. > >> > > > > > >> > > > > Regarding thin clients fix: implementation made by Denis suits > >> better > >> > > to > >> > > > > the very implicit contract that it's better to change API > >> contracts > >> > of > >> > > an > >> > > > > internal IgniteSecurity than of internal GridSecurityProcessor > >> (which > >> > > > > actually mustn't be internal). > >> > > > > > >> > > > > > My approach doesn't require any IEPs, just minor change in > code > >> and > >> > > to > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > >> > > > > > contract. > >> > > > > > >> > > > > Looks like a misuse of #authenticate method to me. It should > >> perform > >> > > > > initial authentication based on credentials (this may include > >> queries > >> > > to > >> > > > > external authentication subsystem, e.g. LDAP). User may want to > >> don't > >> > > > > authenticate thin client on every node (this will increase the > >> number > >> > > of > >> > > > > requests to auth subsystem unless user implicitly implements > >> > > propagation > >> > > > of > >> > > > > thin clients' contexts between nodes and make #authenticate > >> > > cluster-wide > >> > > > > idempotent: first call should perform actual authentication, > next > >> > calls > >> > > > > should retrieve context of already authenticated client). > >> Presence of > >> > > the > >> > > > > separate #securityContext(UUID) highlights that user indeed > should > >> > care > >> > > > > about propagation of thin clients' contexts between the cluster > >> > nodes. > >> > > > > > >> > > > > -- > >> > > > > Ivan > >> > > > > > >> > > > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare < > >> > > v.mith...@cmcmarkets.com > >> > > > > > >> > > > > wrote: > >> > > > > > >> > > > > > Hi Alexei, Denis, > >> > > > > > > >> > > > > > One of the main usecases of thin client authentication is to > be > >> > able > >> > > to > >> > > > > > audit the changes done using the thin client user. > >> > > > > > To enable that : > >> > > > > > We really need to resolve this concern as well : > >> > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > >> > > > > > > >> > > > > > ( Incorrect security subject id is associated with a > cache_put > >> > event > >> > > > > > when the originator of the event is a thin client. ) > >> > > > > > > >> > > > > > Regards, > >> > > > > > Veena > >> > > > > > > >> > > > > > > >> > > > > > -----Original Message----- > >> > > > > > From: Alexei Scherbakov <alexey.scherbak...@gmail.com> > >> > > > > > Sent: 18 March 2020 08:11 > >> > > > > > To: dev <dev@ignite.apache.org> > >> > > > > > Subject: Re: Security Subject of thin client on remote nodes > >> > > > > > > >> > > > > > Denis Garus, > >> > > > > > > >> > > > > > Both variants are capable of solving the thin client security > >> > context > >> > > > > > problem. > >> > > > > > > >> > > > > > My approach doesn't require any IEPs, just minor change in > code > >> and > >> > > to > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > >> > > > > > contract. > >> > > > > > We can add appropriate documentation to emphasize this. > >> > > > > > The argument "fragile" is not very convincing for me. > >> > > > > > > >> > > > > > I think we should collect more opinions before proceeding with > >> IEP. > >> > > > > > > >> > > > > > Considering a fact we actually *may not care* about > >> compatibility > >> > > (I've > >> > > > > > already explained why), I'm thinking of another approach. > >> > > > > > Let's get rid of SecurityContext and use SecuritySubject > >> instead. > >> > > > > > SecurityContext is just a POJO wrapper over SecuritySubject's > >> > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > >> > > > > > It's functionality can be easily moved to SecuritySubject. > >> > > > > > > >> > > > > > What do you think? > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <garus....@gmail.com > >: > >> > > > > > > >> > > > > > > Hello, Alexei! > >> > > > > > > > >> > > > > > > I agree with you if we may not care about compatibility at > >> all, > >> > > then > >> > > > > > > we can solve the problem much more straightforward way. > >> > > > > > > > >> > > > > > > In your case, the method GridSecurityProcessor#authenticate > >> will > >> > > have > >> > > > > > > an implicit contract: > >> > > > > > > [ if actx.subject() != null then > >> > > > > > > returns SecurityContext > >> > > > > > > else > >> > > > > > > do authenticate ] > >> > > > > > > > >> > > > > > > It looks fragile. > >> > > > > > > > >> > > > > > > When we extend the GridSecurityProcessor, there isn't this > >> > problem: > >> > > > > > > we have the explicit contract and can make default > >> implementation > >> > > > that > >> > > > > > > throws an unsupported operation exception to enforcing > >> > > compatibility > >> > > > > > > check. > >> > > > > > > > >> > > > > > > In any case, we need to change GridSecurityProcessor > >> > > implementation. > >> > > > > > > > >> > > > > > > But I think your proposal to try to find a security context > in > >> > the > >> > > > > > > node's attributes first is right for backward compatibility > >> when > >> > > > > > > Ignite users don't use thin clients. > >> > > > > > > > >> > > > > > > Summary: > >> > > > > > > I suggest adding a new method to GridSecurityProcessor > >> because it > >> > > has > >> > > > > > > a clear contract and enforces compatibility check natural > way. > >> > > > > > > > >> > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > >> > > > > > > alexey.scherbak...@gmail.com > >> > > > > > > >: > >> > > > > > > > >> > > > > > > > Denis Garus, > >> > > > > > > > > >> > > > > > > > I've looked at the IEP proposed by you and currently I'm > >> > thinking > >> > > > > > > > it's > >> > > > > > > not > >> > > > > > > > immediately required. > >> > > > > > > > > >> > > > > > > > The problem of missing SecurityContexts of thin clients > can > >> be > >> > > > > > > > solved > >> > > > > > > much > >> > > > > > > > easily. > >> > > > > > > > > >> > > > > > > > Below is the stub of a fix, it requires correct > >> implementation > >> > of > >> > > > > > > > method > >> > > > > > > > > >> > > > > > > > >> > > > > >> org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > >> > > > > > > #authenticatedSubject > >> > > > > > > > by GridSecurityProcessor: > >> > > > > > > > > >> > > > > > > > /** {@inheritDoc} */ > >> > > > > > > > @Override public OperationSecurityContext > >> withContext(UUID > >> > > > > nodeId) > >> > > > > > { > >> > > > > > > > try { > >> > > > > > > > SecurityContext ctx0 = secCtxs.get(nodeId); > >> > > > > > > > > >> > > > > > > > if (ctx0 == null) { > >> > > > > > > > ClusterNode node = > >> > > > > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > >> > > > > > > > .orElseGet(() -> > >> > > > > > > > ctx.discovery().historicalNode(nodeId)); > >> > > > > > > > > >> > > > > > > > // This is a cluster node. > >> > > > > > > > if (node != null) > >> > > > > > > > ctx0 = nodeSecurityContext(marsh, > >> > > > > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > >> > > > > > > > else { > >> > > > > > > > // This is already authenticated thin > >> > client. > >> > > > > > > > SecuritySubject subj = > >> > > > > > > > authenticatedSubject(nodeId); > >> > > > > > > > > >> > > > > > > > assert subj != null : "Subject is null > >> " + > >> > > > > > > > nodeId; > >> > > > > > > > > >> > > > > > > > AuthenticationContext actx = new > >> > > > > > > > AuthenticationContext(); > >> > > > > > > > actx.subject(subj); > >> > > > > > > > > >> > > > > > > > ctx0 = secPrc.authenticate(actx); > >> > > > > > > > } > >> > > > > > > > } > >> > > > > > > > > >> > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > >> > > > > > > > > >> > > > > > > > return withContext(ctx0); > >> > > > > > > > } catch (IgniteCheckedException e) { > >> > > > > > > > throw new IgniteException(e); > >> > > > > > > > } > >> > > > > > > > > >> > > > > > > > The idea is to create a thin client SecurityContext on a > >> node > >> > not > >> > > > > > > > having > >> > > > > > > a > >> > > > > > > > local context using existing SecuritySubject data. > >> > > > > > > > > >> > > > > > > > Method > >> > > > > > > > > >> > > > > > > > >> > > > > >> org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > >> > > > > > > uthenticate > >> > > > > > > > should check for not null SecuritySubject field and just > >> > recreate > >> > > > > > > > SecurityContext using passed info (because it's already > >> > > > > authenticated). > >> > > > > > > > > >> > > > > > > > We have all necessary information in SecuritySubject > >> returned > >> > by > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > >> org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > >> > > > > > > #authenticatedSubject > >> > > > > > > > by GridSecurityProcessor method. > >> > > > > > > > > >> > > > > > > > Because it is internal API, we may not care about > >> > compatibility > >> > > at > >> > > > > > > > all, but nevertheless it is possible to add compatibility > >> check > >> > > in > >> > > > > > > > the method above. If a feature is not supported the > >> operations > >> > > from > >> > > > > > > > thin clients should be forbidden. > >> > > > > > > > > >> > > > > > > > You proposal has the similar problem: if > >> GridSecurityProcessor > >> > > does > >> > > > > > > > not support retriving context for thin clients, such > clients > >> > will > >> > > > > > > > not be able to proceed with operation. > >> > > > > > > > > >> > > > > > > > Still, the cleanup of security API is necessary and should > >> be > >> > > done > >> > > > > > > > in 3.0 > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare < > >> > > > v.mith...@cmcmarkets.com > >> > > > > >: > >> > > > > > > > > >> > > > > > > > > HI , > >> > > > > > > > > > >> > > > > > > > > Created this jira : > >> > > > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > >> > > > > > > > > > >> > > > > > > > > regards, > >> > > > > > > > > Veena. > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > -- > >> > > > > > > > > Sent from: > >> > > > http://apache-ignite-developers.2346864.n4.nabble.com/ > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > -- > >> > > > > > > > > >> > > > > > > > Best regards, > >> > > > > > > > Alexei Scherbakov > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > > > > -- > >> > > > > > > >> > > > > > Best regards, > >> > > > > > Alexei Scherbakov > >> > > > > > ________________________________ > >> > > > > > > >> > > > > > > >> > > > > > Spread bets and CFDs are complex instruments and come with a > >> high > >> > > risk > >> > > > of > >> > > > > > losing money rapidly due to leverage. 70.5% of retail investor > >> > > accounts > >> > > > > > lose money when spread betting and/or trading CFDs with this > >> > > provider. > >> > > > > You > >> > > > > > should consider whether you understand how spread bets and > CFDs > >> > work > >> > > > and > >> > > > > > whether you can afford to take the high risk of losing your > >> money. > >> > > > > > > >> > > > > > Professional clients: Losses can exceed deposits when spread > >> > betting > >> > > > and > >> > > > > > trading CFDs. Countdowns carry a level of risk to your capital > >> as > >> > you > >> > > > > could > >> > > > > > lose all of your investment. These products may not be > suitable > >> for > >> > > all > >> > > > > > clients therefore ensure you understand the risks and seek > >> > > independent > >> > > > > > advice. Invest only what you can afford to lose. > >> > > > > > > >> > > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are > >> > > > authorised > >> > > > > > and regulated by the Financial Conduct Authority in the United > >> > > Kingdom. > >> > > > > CMC > >> > > > > > Markets UK plc and CMC Spreadbet plc are registered in England > >> and > >> > > > Wales > >> > > > > > with Company Numbers 02448409 and 02589529 and with their > >> > registered > >> > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > >> > > > > > > >> > > > > > The content of this e-mail (including any attachments) is > >> strictly > >> > > > > > confidential and is for the sole use of the intended > >> addressee(s). > >> > If > >> > > > you > >> > > > > > are not the intended recipient of this e-mail please notify > the > >> > > sender > >> > > > > > immediately and delete this e-mail from your system. Any > >> > disclosure, > >> > > > > > copying, dissemination or use of its content (including any > >> > > > attachments) > >> > > > > is > >> > > > > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc > >> > reserve > >> > > > the > >> > > > > > right to intercept and monitor the content of the e-mail > >> messages > >> > to > >> > > > and > >> > > > > > from its systems. > >> > > > > > > >> > > > > > E-mails may be interfered with or may contain viruses or other > >> > > defects > >> > > > > for > >> > > > > > which CMC Markets UK plc and CMC Spreadbet plc accept no > >> > > > responsibility. > >> > > > > It > >> > > > > > is the responsibility of the recipient to carry out a virus > >> check > >> > on > >> > > > the > >> > > > > > e-mail and any attachment(s). > >> > > > > > > >> > > > > > This communication is not intended as an offer or solicitation > >> for > >> > > the > >> > > > > > purchase or sale of a financial instrument or as an official > >> > > > confirmation > >> > > > > > of any transaction unless specifically presented as such. > >> > > > > > > >> > > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in > >> England > >> > > and > >> > > > > > Wales with Company Numbers 02448409 and 02589529 and with > their > >> > > > > registered > >> > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > >> > > -- > >> > > > >> > > Best regards, > >> > > Alexei Scherbakov > >> > > > >> > > >> > > >
