[Webkit-unassigned] [Bug 207174] Add WebCrypto LayoutTests to check if PKCS#7 padding is correctly implemented in AES-CBC

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=207174

--- Comment #16 from Jiewen Tan <jiewen_tan at apple.com> ---
(In reply to Tomoki Imai from comment #12)
> (In reply to Jiewen Tan from comment #9)
> > (In reply to Tomoki Imai from comment #8)
> > > (In reply to Jiewen Tan from comment #5)
> > > > (In reply to Tomoki Imai from comment #3)
> > > > > (In reply to Jiewen Tan from comment #2)
> > > > > > Not sure why this is needed? You have to implement PKCS#7 yourself?
> > > > > 
> > > > > I think we need this to validate all the padding is done as expected, and
> > > > > our usage of underlying libraries like CommonCrypt, GCrypt, OpenSSL are
> > > > > correct.
> > > > 
> > > > I don't buy that. Just like you don't write tests to validate WTF before you
> > > > use stuffs from WTF. And I believe if you don't use PKCS#7, the existing
> > > > tests won't pass.
> > > 
> > > Thanks for your reviews!
> > > 
> > > Right, We don't need to validate CommonCrypt, GCrypt, and OpenSSL itself.
> > > I think the PKCS#7 test cases to cover boundary values are needed from the
> > > viewpoint of JavaScript interface, in the other word, black-box testing.
> > > In my opinion, we should validate all the JavaScript interface, regardless
> > > of how they are implemented in platform layer.
> > 
> > Alright, I usually focus on minimum coverage and leave the burden of black
> > box to wpt.
> 
> I'm not sure what "wpt" stands for, but it helps us to understand the
> current situation.
> Is it OK for you to add more LayoutTests to increase the coverage mainly for
> our OpenSSL implementation?
> It should help the other port solid for example bug 207375.

It's definitely ok. However, as I explained before, if you have added this test together with the implementation, then I would immediately know this test is to help OpenSSL's implementation.

> 
> > 
> > > 
> > > > And I believe if you don't use PKCS#7, the existing tests won't pass.
> > > 
> > > Yes, but we didn't have enough test cases to cover boundary values in PKCS#7
> > > now.
> > > 
> > > > > 
> > > > > For OpenSSL backend, we don't have to implement PKCS#7 by ourselves, but we
> > > > > need to setup enough memory for encrypted/decrypted data, which may contains
> > > > > PKCS#7 padding.
> > > > > 
> > > > > Actually we had a bug in calculating the encrypted data length when the
> > > > > length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> > > > > 207176
> > > > 
> > > > Then maybe you should write a test in that patch to help you validate your
> > > > implementation instead of adding new tests like these here.
> > > 
> > > Sure, another option was adding these test cases in bug 207176.
> > > If it's better option than making another patch, then I can move the test
> > > case to bug 207176.
> > > 
> > > The reason why I made a separate bug is that attachment 389613 [details]
> > > affects the port, but bug 207176 only for OpenSSL backend one.
> > 
> > I think it is better to have tests along with the implementation within the
> > same commit. Then, when you look back the commit, you could reason the
> > decision you made from tests.
> > 
> > Given the other commit is landed, you can continue on this one. You should
> > combine them together next time. Otherwise, it is very hard for reviewers to
> > reason the relations between these two patches.
> 
> OK, thanks for letting me know about it. I'll do so from the next time.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200210/9b1c56e3/attachment.htm>