fix deadlock in crypto setup when it is closed while handling a message

[marten-seemann]

Fixes #2737.

@egonelbre's analysis (option B) in #2737 (comment) was right.

Unfortunately, I didn't manage to write an integration test that would trigger the bug, so I'm not 100% sure that this fix actually works under all circumstances.

[codecov]

Codecov Report

Merging #2802 into master will decrease coverage by 0.04%.
The diff coverage is 61.54%.

@@            Coverage Diff             @@
##           master    #2802      +/-   ##
==========================================
- Coverage   86.02%   85.98%   -0.04%     
==========================================
  Files         133      133              
  Lines       12063    12068       +5     
==========================================
  Hits        10376    10376              
- Misses       1355     1360       +5     
  Partials      332      332              

Impacted Files	Coverage Δ
internal/handshake/crypto_setup.go	65.46% <61.54%> (-0.66%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93e733a...9ce5426. Read the comment docs.

[egonelbre]

I often try to separate the concurrency bits and channel handling into a separate struct, such that it can be separately tested... however seems bit difficult here.

If you could hook into h.conn.Handshake somehow, then you could call SendAlert from it. And concurrently call Close.

[egonelbre]

On line 488, is there a possibility the go func() { <-h.isReadingHandshakeMessage gets stuck somehow?

I noticed that it doesn't have an obvious (to me) condition for shutting down on close.

[marten-seemann]

Good catch. That one should probably be protected by a select statement as well.

[marten-seemann]

@egonelbre Yes, that would be nice, but the way the TLS library is structured doesn't allow that.
Ideally, the library would expose a HandleHandshakeMessage() (response []byte, err error) method. Instead, we need to call Handshake, which basically does the following:

func Handshake() {
    for {
          msg := cryptosetup.ReadHandshakeMessage()
          if err := handleMsg(msg); err != nil {
               cryptosetup.SendAlert()
               break
          }
          if handshake complete {
               break
          }
    }
}